Todd Lipcon has posted comments on this change. Change subject: Get and use Kudu from the toolchain by default ......................................................................
Patch Set 3: (11 comments) 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 file? Dan's suggestion of doing an #ifndef #error thing seemed good to make it clearer to readers 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, should this be less specific and say it's not supported in this version of Impala? Or do we expect that 5.8 will actually ship with whatever version of Kudu client was available at code freeze? 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 suite, rather than in each test method 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 of the build... is this the norm in Impala? 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 allocated by the OS, and it's quite possible there are already other things listening. 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? also should have "" around "$KUDU_BUILD_DIR" so that it doesn't explode if you have spaces in the path like on macs Line 5: if $USE_KUDU_DEBUG_BUILD; then do you require $USE_KUDU_DEBUG_BUILD to be passed in? Maybe ${USE_KUDU_DEBUG_BUILD:-false} is better so that if it's not passed in you get something reasonable here? documentation at the top of the file of the required environments would improve this. 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? Line 9: do_start "$KUDU_BIN_DIR"/kudu-master \ same: documenting required environment variables and potentially using something like: if [ ! -d "$KUDU_BIN_DIR"]; then echo must pass KUDU_BIN_DIR exit 1 fi ... at the top Line 11: -vmodule IBelongToTheMiniCluster huh? this isn't a valid module. is the purpose here to simplify grepping? 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 -- 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
