Tim Armstrong 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) 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 I ended up going down this path because the set of public-facing ports exposed by each daemon is different. I suppose we could expose the superset of all ports, but that seems to be not the way things are usually done in Docker - I think? 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 entrypoi Fair point. Converted to a daemon_entrypoint.sh script 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., / Done 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 Done 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 de I removed the arg parsing so this is not longer present. 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 act Yeah I think I made this more general than necessary in anticipation of requiring build contexts for the other containers. Removed. 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 general Done 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 scrip I did the rewrite. It wasn't quite mechanical but wasn't particularly tricky. I removed the argument parsing and assumed fixed paths for now. -- 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: Thu, 13 Dec 2018 02:57:03 +0000 Gerrit-HasComments: Yes
