Casey Ching has posted comments on this change. Change subject: Get and use Kudu from the toolchain by default ......................................................................
Patch Set 3: (11 comments) Also includes a fix to avoid checking that kudu is started when it is not supported. http://gerrit.cloudera.org:8080/#/c/1985/3/be/src/exec/kudu-scan-node-test.cc File be/src/exec/kudu-scan-node-test.cc: Line 15: #ifdef KUDU > isn't this already taken care of at the cmake level by not building this fi Done http://gerrit.cloudera.org:8080/#/c/1985/3/common/thrift/generate_error_codes.py File common/thrift/generate_error_codes.py: Line 214: ("KUDU_NOT_SUPPORTED_ON_OS", 69, "Kudu is not supported on this operating system.") > do we plan to disable kudu on our mainline CDH builds as well? In that case I don't know if that has been decided yet. Alex and I just talked about this a little and releasing with a version of the Kudu client is what we'd like to do (we haven't talked to Marcel/Silvius yet though). Are you ok with that? http://gerrit.cloudera.org:8080/#/c/1985/3/fe/src/test/java/com/cloudera/impala/planner/KuduPlannerTest.java File fe/src/test/java/com/cloudera/impala/planner/KuduPlannerTest.java: Line 29: TestUtils.assumeKuduIsSupported(); > I think you can put this in a @Before so that it affects the whole test sui Done http://gerrit.cloudera.org:8080/#/c/1985/3/fe/src/test/java/com/cloudera/impala/testutil/TestUtils.java File fe/src/test/java/com/cloudera/impala/testutil/TestUtils.java: Line 242: Assume.assumeTrue("true".equals(System.getenv("KUDU_IS_SUPPORTED"))); > it seems goofy to use the runtime environment here rather than some aspect I'm not sure if this is done elsewhere. Do you know of some way have this determined at build time? http://gerrit.cloudera.org:8080/#/c/1985/3/testdata/cluster/admin File testdata/cluster/admin: Line 67: KUDU_TS_RPC_FREE_PORT_START=37000 > I wouldn't recommend using ports above 32000 because they're ephemeral ones Done http://gerrit.cloudera.org:8080/#/c/1985/3/testdata/cluster/node_templates/cdh5/etc/init.d/kudu-common File testdata/cluster/node_templates/cdh5/etc/init.d/kudu-common: Line 1: if [[ -n $KUDU_BUILD_DIR ]]; then > license header and shebang? The quotes are there now. The file is meant to be sourced so no shebang was added. A bunch of other files don't have the license either. If it's really needed that can be added in bulk. Do you happen to know what the apache policy is there? Does every file need the license? Line 5: if $USE_KUDU_DEBUG_BUILD; then > do you require $USE_KUDU_DEBUG_BUILD to be passed in? Maybe ${USE_KUDU_DEBU Added a comment at the top http://gerrit.cloudera.org:8080/#/c/1985/3/testdata/cluster/node_templates/cdh5/etc/init.d/kudu-master File testdata/cluster/node_templates/cdh5/etc/init.d/kudu-master: Line 2: > set -e? Done Line 9: do_start "$KUDU_BIN_DIR"/kudu-master \ > same: documenting required environment variables and potentially using some That var is set by kudu-common so I left it as is. (We could assert the common script does its job but that seems excessively noisy.) Line 11: -vmodule IBelongToTheMiniCluster > huh? this isn't a valid module. is the purpose here to simplify grepping? Ha, added a comment. Ya another script uses that to identify processes in case pid files are deleted. http://gerrit.cloudera.org:8080/#/c/1985/3/testdata/cluster/node_templates/cdh5/etc/init.d/kudu-tserver File testdata/cluster/node_templates/cdh5/etc/init.d/kudu-tserver: Line 2: > same feedback on this file as the kudu-master script Done -- To view, visit http://gerrit.cloudera.org:8080/1985 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3db88cbd27f2ea2394f011bc8d1face37411ed58 Gerrit-PatchSet: 3 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Casey Ching <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Casey Ching <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
