Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/15393 )
Change subject: [docker] Add an Impala quickstart image ...................................................................... Patch Set 4: (15 comments) http://gerrit.cloudera.org:8080/#/c/15393/4/docker/Dockerfile File docker/Dockerfile: http://gerrit.cloudera.org:8080/#/c/15393/4/docker/Dockerfile@356 PS4, Line 356: # Build Impala : RUN source bin/impala-config.sh \ : && ./buildall.sh -release -noclean -notests \ : && docker/setup_build_context.py > Is this able to succeed outside of the internal cloudera network? I rememb This is pulling an Apache source release, so it should be able to complete outside of Cloudera. If it can't I will raise that with the Apache Impala community. In the mean time I can build and publish the image from within if there is an issue. I also ran a full build on Centos 7 when testing this. I think Impala development is mean to be done on Ubuntu 16 but builds and packaging can be done on many OSs. There are a couple issues with `setup_build_context.py` and `daemon_entrypoint.sh` that I will address in a follow up patch. http://gerrit.cloudera.org:8080/#/c/15393/4/docker/Dockerfile@474 PS4, Line 474: > nit: will this extra spacing end up into a too-long space in the result ima It does include the extra spaces. I will fixes all these. http://gerrit.cloudera.org:8080/#/c/15393/4/docker/Dockerfile@478 PS4, Line 478: $MAINTAINER > What if MAINTAINER's value contains spaces? That is okay. Docker handles this correctly. http://gerrit.cloudera.org:8080/#/c/15393/4/docker/Dockerfile@482 PS4, Line 482: $VCS_URL > Does it work as expected if VCS_URL contains '#' or something like that? M Yes this is just a text tag. Docker handles this correctly. http://gerrit.cloudera.org:8080/#/c/15393/4/docker/bootstrap-java-env.sh File docker/bootstrap-java-env.sh: http://gerrit.cloudera.org:8080/#/c/15393/4/docker/bootstrap-java-env.sh@27 PS4, Line 27: > Enable 'pipefail' option since to spot issues with VERSION_NAME early? See my comment on the entrypoint script. http://gerrit.cloudera.org:8080/#/c/15393/4/docker/bootstrap-java-env.sh@39 PS4, Line 39: /tmp/* /var/tmp/* > I'm not sure this is safe: in /tmp and /var/tmp might be various files whic This is a part of bootstrapping and building a docker image. It's not meant to run on anyones machine. This is fairly common practice in Docker scripts. http://gerrit.cloudera.org:8080/#/c/15393/4/docker/bootstrap-maven-env.sh File docker/bootstrap-maven-env.sh: http://gerrit.cloudera.org:8080/#/c/15393/4/docker/bootstrap-maven-env.sh@29 PS4, Line 29: apache-maven-3.6.3-bin.tar.gz > Create variable for this and re-use it everywhere in the script? Done http://gerrit.cloudera.org:8080/#/c/15393/4/docker/bootstrap-maven-env.sh@31 PS4, Line 31: z > nit: 'z' isn't necessary here, can be omitted Done http://gerrit.cloudera.org:8080/#/c/15393/4/docker/bootstrap-maven-env.sh@32 PS4, Line 32: /usr/local/bin > What if /usr/local/bin didn't exist prior to this invocation? I haven't seen a docker base OS image where it doesn't yet. I would favor simplicity over defensiveness here given docker is meant for reproducible build. http://gerrit.cloudera.org:8080/#/c/15393/4/docker/bootstrap-maven-env.sh@33 PS4, Line 33: rm > rm -f ? Done http://gerrit.cloudera.org:8080/#/c/15393/4/docker/impala-entrypoint.sh File docker/impala-entrypoint.sh: http://gerrit.cloudera.org:8080/#/c/15393/4/docker/impala-entrypoint.sh@24 PS4, Line 24: set -e > Maybe, set the 'pipefail' option as well to catch the issues with JAVA_HOME I am not sure what could fail in a bad or unclear way. Especially considering this is a docker image where we control the environment. To maintain minimalism I think I would like to leave it out. http://gerrit.cloudera.org:8080/#/c/15393/4/docker/impala-entrypoint.sh@46 PS4, Line 46: which > Built the Impala environment with centos:7. Upon going through the quicksta I fixed this, but there are a couple more issues in `daemon_entrypoint.sh` and `setup_build_context.py` that I will address in a follow on patch. For now, this should be okay given we will only publish the Ubuntu image. http://gerrit.cloudera.org:8080/#/c/15393/4/docker/impala/etc/hadoop/conf/core-site.xml File docker/impala/etc/hadoop/conf/core-site.xml: http://gerrit.cloudera.org:8080/#/c/15393/4/docker/impala/etc/hadoop/conf/core-site.xml@26 PS4, Line 26: <name>dfs.client.read.shortcircuit</name> : <value>true</value> > Could you add a comment explaining the reason for this customization? I removed it since it may not be needed. http://gerrit.cloudera.org:8080/#/c/15393/4/docker/impala/etc/hadoop/conf/core-site.xml@29 PS4, Line 29: <property> : <name>hadoop.proxyuser.root.hosts</name> : <value>*</value> : </property> : <property> : <name>hadoop.proxyuser.root.groups</name> : <value>*</value> : </property> : <property> : <name>hadoop.proxyuser.impala.hosts</name> : <value>*</value> : </property> : <property> : <name>hadoop.proxyuser.impala.groups</name> : <value>*</value> : </property> > Do you mind adding a small comment to explain why the defaults are not good Done http://gerrit.cloudera.org:8080/#/c/15393/4/docker/impala/etc/hive/conf/hive-site.xml File docker/impala/etc/hive/conf/hive-site.xml: http://gerrit.cloudera.org:8080/#/c/15393/4/docker/impala/etc/hive/conf/hive-site.xml@38 PS4, Line 38: <name>hive.metastore.schema.verification</name> : <value>false</value> > nit: do you mind adding a comment to explain why it's crucial to have this It was left over from when I was using auto schema generation. I will remove it. -- To view, visit http://gerrit.cloudera.org:8080/15393 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2d88bcb52f793c0fc83c4f84a9b56c5b961e58d5 Gerrit-Change-Number: 15393 Gerrit-PatchSet: 4 Gerrit-Owner: Grant Henke <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 12 Mar 2020 14:01:12 +0000 Gerrit-HasComments: Yes
