Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15393 )
Change subject: [docker] Add an Impala quickstart image ...................................................................... Patch Set 4: (10 comments) 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? 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 which are created and used by other components of the OS (e.g., there might be Kerberos cache files, SSH pipes, etc.) 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? 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 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? http://gerrit.cloudera.org:8080/#/c/15393/4/docker/bootstrap-maven-env.sh@33 PS4, Line 33: rm rm -f ? 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 assignment earlier, if any? 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? 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 enough and these properties are set to these custom values? 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 customization here? Why the default value isn't good enough? -- 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: Wed, 11 Mar 2020 21:59:12 +0000 Gerrit-HasComments: Yes
