Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12173 )
Change subject: [docker] Add an initial docker build ...................................................................... Patch Set 3: (17 comments) http://gerrit.cloudera.org:8080/#/c/12173/2/docker/Dockerfile File docker/Dockerfile: http://gerrit.cloudera.org:8080/#/c/12173/2/docker/Dockerfile@71 PS2, Line 71: # TODO: Remove build dir too? > Right, that's the issue I ran into and I didn't try to fix it yet. Being ab I removed all but the llvm directory. http://gerrit.cloudera.org:8080/#/c/12173/3/docker/Dockerfile File docker/Dockerfile: http://gerrit.cloudera.org:8080/#/c/12173/3/docker/Dockerfile@32 PS3, Line 32: ARG BASE_OS=ubuntu:xenial > I'm curious, why do we default to ubuntu:xenial? In patch set one we used c ubuntu seems to be a more common choice among ecosystem images (impala for example) so I set it as the default now that it is supported. My next patch update adds more support. http://gerrit.cloudera.org:8080/#/c/12173/3/docker/Dockerfile@57 PS3, Line 57: # : # ---- Thirdparty ---- : # Builds an image that has Kudu's thirdparty dependencies built. : # This is done in it's own stage so that docker can cache it and only : # run it when thirdparty has changes. : # > While not necessary, maybe we should consider installing ccache/ninja (thou I had considered this but wanted to keep things minimal. ccache and ninja are likely most useful on the kudu-build image which I imagine would be used as psuedo dev/experimentation containers. Otherwise the standard build tools/process will be used and the cache will be fresh each build unless we setup volumes. Lets address this in a follow up change. http://gerrit.cloudera.org:8080/#/c/12173/3/docker/README.adoc File docker/README.adoc: http://gerrit.cloudera.org:8080/#/c/12173/3/docker/README.adoc@72 PS3, Line 72: : === kudu-master : A runtime image with kudu-master as the ENTRYPOINT. : Uses the kudu-runtime image as a base. : : === kudu-tserver : A runtime image with kudu-tserver as the ENTRYPOINT. : Uses the kudu-runtime image as a base. > You might not have run into this if your docker version is new, but it migh I think I workaround this by setting `--use_hybrid_clock=false` so that `ntp` isn't needed. That is something that will be addressed when those TODO's are done. http://gerrit.cloudera.org:8080/#/c/12173/3/docker/bootstrap-env.sh File docker/bootstrap-env.sh: http://gerrit.cloudera.org:8080/#/c/12173/3/docker/bootstrap-env.sh@28 PS3, Line 28: # Install the prerequisite libraries, if they are not installed. > I know it's not in the upstream installation docs, but it's probably worth I added this as a todo. This patch is large and will have many improvements going forward. I have found and included a few things that are not in the upstream docs and planned to add them at some point. Alternatively we could point people to this script to setup their environment, though I am not sure we want to mix it's purpose. http://gerrit.cloudera.org:8080/#/c/12173/3/docker/bootstrap-env.sh@58 PS3, Line 58: \ > drop Why drop? Not sure what this means? http://gerrit.cloudera.org:8080/#/c/12173/3/docker/bootstrap-env.sh@36 PS3, Line 36: autoconf \ : automake \ : cyrus-sasl-devel \ : cyrus-sasl-gssapi \ : cyrus-sasl-plain \ : flex \ : gcc \ : gcc-c++ \ : gdb \ : git \ : java-1.8.0-openjdk-devel \ : krb5-server \ : krb5-workstation \ : libtool \ : make \ : openssl-devel \ : patch \ : pkgconfig \ : redhat-lsb-core \ : rsync \ : unzip \ : vim-common \ : whic > nit here and below: add an extra indent for the continuation of the command Done http://gerrit.cloudera.org:8080/#/c/12173/3/docker/docker-build.sh File docker/docker-build.sh: http://gerrit.cloudera.org:8080/#/c/12173/3/docker/docker-build.sh@37 PS3, Line 37: --build-arg MAINTAINER="Apache Kudu<d...@kudu.apache.org>" > nit: I think there should be a space between the name and the email Done http://gerrit.cloudera.org:8080/#/c/12173/3/docker/docker-build.sh@45 PS3, Line 45: docker build "${BUILD_ARGS[@]}" -f $ROOT/docker/Dockerfile --target kudu-base -t kudu-base $ROOT > I believe the tag should be kudu:* instead of kudu-* and each image should One of my open follow up items listed in the commit message is "Add an upload script with good tagging." I have a lot of options/thoughts on tagging, but given others might too I wanted to handle that in a separate patch. I think tagging and uploading thirdparty is useful because you can pull a pre-built thirdparty instead of building it yourself. The same is true for the kudu-build. You can pull it and use it to copy any artifacts out for a derived image. http://gerrit.cloudera.org:8080/#/c/12173/3/docker/kudu-entrypoint.sh File docker/kudu-entrypoint.sh: http://gerrit.cloudera.org:8080/#/c/12173/3/docker/kudu-entrypoint.sh@27 PS3, Line 27: resolved > ... reached by their DNS name ... This was an easy way to get past the failures mentioned in the doc below. We can improve it going forward. http://gerrit.cloudera.org:8080/#/c/12173/3/docker/kudu-entrypoint.sh@51 PS3, Line 51: /var/lib/kudu/master > Turn into a variable to re-use elsewhere? I don't want to over engineer this yet. It's doesn't have a lot of real use and will surely evolve in my follow on patches. http://gerrit.cloudera.org:8080/#/c/12173/3/docker/kudu-entrypoint.sh@52 PS3, Line 52: $KUDU_FLAGS > KUDU_FLAGS is common for both kudu-master and kudu-tservers. Does it make Based on current projected usage, in docker compose files, it's defined per service so this is okay. It can change over time if needed. This is experimental right now. good, point. I can move it to the end. http://gerrit.cloudera.org:8080/#/c/12173/3/docker/kudu-entrypoint.sh@55 PS3, Line 55: /opt/kudu/www > Is it guaranteed to exist? Or that doesn't matter? same as other locations. http://gerrit.cloudera.org:8080/#/c/12173/3/docker/kudu-entrypoint.sh@62 PS3, Line 62: /var/lib/kudu/tserver > Turn into a variable to re-use elsewhere? same as above. http://gerrit.cloudera.org:8080/#/c/12173/3/docker/kudu-entrypoint.sh@66 PS3, Line 66: /opt/kudu/www > Is it guaranteed to exist? Or that doesn't matter? yes it's guaranteed. The kudu-runtime image places it there. http://gerrit.cloudera.org:8080/#/c/12173/2/thirdparty/build-thirdparty.sh File thirdparty/build-thirdparty.sh: http://gerrit.cloudera.org:8080/#/c/12173/2/thirdparty/build-thirdparty.sh@248 PS2, Line 248: # Install Hadoop, Hive, and Sentry by copying their source directories into $PREFIX/opt. : # Copying allows the source directory to be removed once thirdparty is built. > Hmm, that's a fair amount of space relative to the size of the thirdparty i Done http://gerrit.cloudera.org:8080/#/c/12173/3/thirdparty/build-thirdparty.sh File thirdparty/build-thirdparty.sh: http://gerrit.cloudera.org:8080/#/c/12173/3/thirdparty/build-thirdparty.sh@252 PS3, Line 252: rm -rf $PREFIX/opt/hadoop > nit: I'm not sure removing data automatically is a good thing to do. Maybe I undid this change. -- To view, visit http://gerrit.cloudera.org:8080/12173 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I95497b39e47f07301be75cbadd814656c7e2ea42 Gerrit-Change-Number: 12173 Gerrit-PatchSet: 3 Gerrit-Owner: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Attila Bukor <abu...@apache.org> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 23 Jan 2019 20:17:02 +0000 Gerrit-HasComments: Yes
