Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12173 )
Change subject: [docker] Add an initial docker build ...................................................................... Patch Set 3: (11 comments) Just skimmed through. It's great that we are about to have that dockerized stuff soon! 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@30 PS3, Line 30: if [[ -n $(which yum) ]]; why not just if $(which yum); then ... fi ? http://gerrit.cloudera.org:8080/#/c/12173/3/docker/bootstrap-env.sh@58 PS3, Line 58: \ drop 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 line 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@22 PS3, Line 22: KUDU_FLAGS Do those come from the environment? Or that's about modifying this particular script and populating KUDU_FLAGS with something? If the latter, consider introducing the KUDU_FLAGS var with default value "" (i.e. empty). http://gerrit.cloudera.org:8080/#/c/12173/3/docker/kudu-entrypoint.sh@27 PS3, Line 27: resolved ... reached by their DNS name ... This also verifies that network interfaces of master hosts are up and running. BTW, is it a requirement to have master hosts up and running? If not, why to use ping instead of DNS-related utils? 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? 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 sense to separate those? Also, if KUDU_FLAGS are here for customization, maybe make them last so it would be possible to override some options specified by this script? 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? Also, consider turning it into a variable to re-use elsewhere in the script. 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? 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? 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, just move the old dir X into X.$(date)? -- 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 <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 23 Jan 2019 18:16:25 +0000 Gerrit-HasComments: Yes
