[ISSUE-48][FEATURE] Add Uniffle Dockerfile#132
Conversation
wangao1236
changed the title
roryqi
changed the title
roryqi
changed the title
roryqi
changed the title
|
Could you remove the file https://github.com/apache/incubator-uniffle/blob/master/deploy/kubernetes/docker/.gitkeep? Because the directory |
| @@ -0,0 +1,29 @@ | |||
| FROM openjdk:8-jdk-slim | |||
There was a problem hiding this comment.
I suggest to use other general-purpose images, such as alpine or centos.
Many companies may have their own JDK choices, so we should treat JDK as 3rd party lib hadoop.
There was a problem hiding this comment.
It's ok to use openjdk. You can see Spark's Dockerfile https://github.com/apache/spark/blob/b481ed36d129982bdd5aedd0d5864dd954dad006/resource-managers/kubernetes/docker/src/main/dockerfiles/spark/Dockerfile#L19
There was a problem hiding this comment.
I think we can use dependency_cache to save our build time
for example:
FROM openjdk:8-jdk-slim as dependency_cache
There was a problem hiding this comment.
Seems 8-jdk-slim image is based on Debian stable, which is fine.
Can we use 8-jre-slim here?
There was a problem hiding this comment.
Seems
8-jdk-slimimage is based on Debian stable, which is fine. Can we use8-jre-slimhere?
What's your reason?
There was a problem hiding this comment.
Seems
8-jdk-slimimage is based on Debian stable, which is fine. Can we use8-jre-slimhere?What's your reason?
It's smaller.
There was a problem hiding this comment.
I think we can use dependency_cache to save our build time
for example: FROM openjdk:8-jdk-slim as dependency_cache
It's great. Could you raise a follow-up pr which uses dependency_cache?
There was a problem hiding this comment.
Seems
8-jdk-slimimage is based on Debian stable, which is fine. Can we use8-jre-slimhere?What's your reason?
It's smaller.
It may need more tests. If 8-jdk-slim is fine, we can choose it first. When our e2e test is ready, wen can use e2e test to verify the 8-jre-slim.
wangao1236
force-pushed
the
build-add-dockerfile
branch
2 times, most recently
from
1a0a973 to
f776ce7
Compare
| rss-*.tgz | ||
|
|
||
| hadoop-*.tar.gz | ||
| /deploy/kubernetes/hadoopconfig/* |
There was a problem hiding this comment.
Why do we use absolute path here? if hadoopconfig is useful, why do we add it to .gitignore file?
zuston
left a comment
zuston
left a comment
There was a problem hiding this comment.
Overall LGTM. Left some minor comments
| IMAGE=$REGISTRY/rss-server:$RSS_VERSION | ||
|
|
||
| echo "building image: $IMAGE" | ||
| docker build -t "$IMAGE" \ |
There was a problem hiding this comment.
Using LABEL to add more metadata, like build-time/author/branch/commit-id?
|
|
||
| if [ "$SERVICE_NAME" == "coordinator" ];then | ||
|
|
||
| bash ${basedir}/bin/start-coordinator.sh & |
There was a problem hiding this comment.
Directly exit when executing startup script failed.
bash ${basedir}/bin/start-coordinator.sh || exit 1
There was a problem hiding this comment.
Could you give me your reason?
There was a problem hiding this comment.
start.sh will monitor the process existence, if the coordinator startup failed, we should exit directly, instead of waiting the non-existence PID and then exit 0
There was a problem hiding this comment.
We kill server or coordinator, the process will not exit immediately sometimes. The process will occupy the port, we should wait for the port for a while.
| RUN tar -zxvf /data/rssadmin/hadoop-${HADOOP_VERSION}.tar.gz -C /data/rssadmin | ||
| RUN mv /data/rssadmin/hadoop-${HADOOP_VERSION} /data/rssadmin/hadoop | ||
| RUN rm -rf /data/rssadmin/hadoop-${HADOOP_VERSION}.tar.gz | ||
| COPY hadoopconfig/ /data/rssadmin/hadoop/etc/hadoop |
There was a problem hiding this comment.
What files in hadoopconfig folder? core-site/hdfs-site.xml?
| ARG HADOOP_VERSION | ||
| ARG RSS_VERSION | ||
|
|
||
| RUN apt-get update && apt-get install -y zlib1g zlib1g-dev lzop lsof netcat dnsutils less procps iputils-ping \ |
There was a problem hiding this comment.
Do we need to consider kerberos? If yes, we need to setup kerberos client.
Maybe we shouldn't consider this in early version.
There was a problem hiding this comment.
We shouldn't consider the kerberos dependency here. It's ok for us if there are no big conflicts, it will be convenient to modify Dockerfile after we merge #53
wangao1236
force-pushed
the
build-add-dockerfile
branch
from
f776ce7 to
d85a10e
Compare
wangao1236
force-pushed
the
build-add-dockerfile
branch
from
d85a10e to
7da0c3e
Compare
roryqi
left a comment
roryqi
left a comment
There was a problem hiding this comment.
LGTM, @zuston @frankliee @kaijchen @czy006 Do you have any other suggestion?
6 participants
What changes were proposed in this pull request?
To solve issue #48 ，I add the Dockerfile for Uniffle.
Why are the changes needed?
Support K8S Operator
Does this PR introduce any user-facing change?
Yes, we will add the document later
How was this patch tested?
Manual test