Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/12074 )
Change subject: IMPALA-7948: part 1: initial docker container build ...................................................................... Patch Set 4: (8 comments) Overall structure seems fine. See some minor comments below. Thanks! http://gerrit.cloudera.org:8080/#/c/12074/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12074/4//COMMIT_MSG@19 PS4, Line 19: images called "impala_base", "impalad", "catalogd" and : "statestored": : : ninja -j $IMPALA_BUILD_THREADS docker_images I think it's acceptable to have only one image which takes arguments. Doesn't matter much one way or another, but it makes life a bit simpler. http://gerrit.cloudera.org:8080/#/c/12074/4/docker/impala_base/Dockerfile File docker/impala_base/Dockerfile: http://gerrit.cloudera.org:8080/#/c/12074/4/docker/impala_base/Dockerfile@28 PS4, Line 28: ENV LD_LIBRARY_PATH=/usr/lib/jvm/java-8-openjdk-amd64/jre/lib/amd64/ : ENV LD_LIBRARY_PATH=${LD_LIBRARY_PATH}:/usr/lib/jvm/java-8-openjdk-amd64/jre/lib/amd64/server/ : ENV LD_LIBRARY_PATH=${LD_LIBRARY_PATH}:/etc/kudu/release/lib You're already using /etc/impala/bin/run_with_classpath.sh in your entrypoint. It seems like it may make sense to encapsulate the necessary LD_LIBRARY_PATH in an entrypoint shell script rather than via Docker. http://gerrit.cloudera.org:8080/#/c/12074/4/docker/impala_base/Dockerfile@31 PS4, Line 31: ENV IMPALA_HOME=/etc/impala It's not a huge deal, but this feels more like "/opt/impala" to me. i.e., /etc is just for configs, and it's weird to put the binaries in there. http://gerrit.cloudera.org:8080/#/c/12074/4/docker/run_with_classpath.sh File docker/run_with_classpath.sh: http://gerrit.cloudera.org:8080/#/c/12074/4/docker/run_with_classpath.sh@33 PS4, Line 33: exit $? This is usually going to exit gracefully because it's the exit code of the cat. (Unless the log file doesn't exist...). I think you need to capture the return code of line 28. http://gerrit.cloudera.org:8080/#/c/12074/4/docker/setup_build_context.sh File docker/setup_build_context.sh: http://gerrit.cloudera.org:8080/#/c/12074/4/docker/setup_build_context.sh@32 PS4, Line 32: echo "Set up a docker build context for the specified Dockerfile with the impala"\ "Assembles the artifacts required..." as in line 19 is clearer than this description. ("build context" by itself isn't obvious.) Perhaps use that text instead? http://gerrit.cloudera.org:8080/#/c/12074/4/docker/setup_build_context.sh@34 PS4, Line 34: echo " -d <dockerfile> - set the Dockerfile for the build (required)" I think you only ever use this with the "impala_base" dockerfile. Do we actually need to make this configurable? http://gerrit.cloudera.org:8080/#/c/12074/4/docker/setup_build_context.sh@35 PS4, Line 35: echo " [-o <directory>] - set the output directory (defaults to"\ Same here. I don't think build directories in our cmake targets are generally configurable; should we just make this always use the same locations? http://gerrit.cloudera.org:8080/#/c/12074/4/docker/setup_build_context.sh@30 PS4, Line 30: usage() { : echo "setup_build_context.sh <options>" : echo "Set up a docker build context for the specified Dockerfile with the impala"\ : "build artifacts required to produce single-process containers." : echo " -d <dockerfile> - set the Dockerfile for the build (required)" : echo " [-o <directory>] - set the output directory (defaults to"\ : "docker/build_context)" : } : : # parse command line options : while [ -n "$*" ] : do : case "$1" in : -o) : OUTPUT_DIR="${2-}" : shift; : ;; : -d) : DOCKERFILE="${2-}" : if [[ "$(basename $DOCKERFILE)" != "Dockerfile" ]] : then : echo "$DOCKERFILE must be called Dockerfile" : usage : exit 1 : fi : shift; : ;; : -help|*) : usage : exit 1 : ;; : esac : shift; : done This is all fine, but my general rule of thumb is that I regret shell scripts this long. You'll be happier with python argparse, likely. -- To view, visit http://gerrit.cloudera.org:8080/12074 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifea707aa3cc23e4facda8ac374160c6de23ffc4e Gerrit-Change-Number: 12074 Gerrit-PatchSet: 4 Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Wed, 12 Dec 2018 20:36:42 +0000 Gerrit-HasComments: Yes
