Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/11410 )
Change subject: IMPALA-6249: Expose several build flags via web UI ...................................................................... Patch Set 11: (11 comments) http://gerrit.cloudera.org:8080/#/c/11410/11/be/src/common/global-flags.cc File be/src/common/global-flags.cc: http://gerrit.cloudera.org:8080/#/c/11410/11/be/src/common/global-flags.cc@164 PS11, Line 164: const static int32 default_kudu_client_rpc_timeout_ms = I think it's better to copy the full line twice: #ifdef ... static const a = ... #else static const a = ... #endif also nit: ~/i1(master) ?$ git grep "static const" be | wc -l 1100 ~/i1(master) ?$ git grep "const static" be | wc -l 48 ~/i1(master) ?$ http://gerrit.cloudera.org:8080/#/c/11410/11/be/src/util/debug-util.h File be/src/util/debug-util.h: http://gerrit.cloudera.org:8080/#/c/11410/11/be/src/util/debug-util.h@105 PS11, Line 105: For some reason Do we know the reason? Can we find out? http://gerrit.cloudera.org:8080/#/c/11410/11/be/src/util/default-path-handlers.cc File be/src/util/default-path-handlers.cc: http://gerrit.cloudera.org:8080/#/c/11410/11/be/src/util/default-path-handlers.cc@245 PS11, Line 245: const char* is_ndebug = I think it's more readable to write the const char* ... line twice, see my comment elsewhere. http://gerrit.cloudera.org:8080/#/c/11410/8/tests/common/environ.py File tests/common/environ.py: http://gerrit.cloudera.org:8080/#/c/11410/8/tests/common/environ.py@108 PS8, Line 108: : > Yeah, but flake8 gives me a warning saying "undefined name 'validate_build_ Oh, right. You have to use cls.get_... http://gerrit.cloudera.org:8080/#/c/11410/8/tests/common/environ.py@234 PS8, Line 234: """ > Changed to is_remote_cluster I don't feel strongly, but it seems to me that there's some missing semantics, too. remote_cluster refers to the fact that the whole cluster under test is not available locally, and Impalad usually refers to the daemon (lots of this is historic). Maybe ImpalaTestClusterProperties or something similar would be an option, too. Please pick what you like best. http://gerrit.cloudera.org:8080/#/c/11410/8/tests/common/environ.py@242 PS8, Line 242: > Changed it to build_flavor. Required some re-factoring to other classes too Thanks for taking the effort, I think it's much better. http://gerrit.cloudera.org:8080/#/c/11410/11/tests/common/environ.py File tests/common/environ.py: http://gerrit.cloudera.org:8080/#/c/11410/11/tests/common/environ.py@115 PS11, Line 115: else LinkTypes.DYNAMIC nit: check indent http://gerrit.cloudera.org:8080/#/c/11410/11/tests/common/environ.py@200 PS11, Line 200: ImpalaBuildFlavors.CODE_COVERAGE_RELEASE) nit: indent http://gerrit.cloudera.org:8080/#/c/11410/11/tests/common/skip.py File tests/common/skip.py: http://gerrit.cloudera.org:8080/#/c/11410/11/tests/common/skip.py@145 PS11, Line 145: build nit: cluster http://gerrit.cloudera.org:8080/#/c/11410/11/tests/run-tests.py File tests/run-tests.py: http://gerrit.cloudera.org:8080/#/c/11410/11/tests/run-tests.py@42 PS11, Line 42: INVALID_TEST_DIRS = ['aux_parquet_data_load', 'test-hive-udfs', 'comparison', 'benchmark', I think this is better and obviously less error-prone then the previous solution. Please check with David though, who's the assignee of IMPALA-4417. http://gerrit.cloudera.org:8080/#/c/11410/11/tests/webserver/test_web_pages.py File tests/webserver/test_web_pages.py: http://gerrit.cloudera.org:8080/#/c/11410/11/tests/webserver/test_web_pages.py@93 PS11, Line 93: assert not build_flags["cmake_build_type"] in ["release"] nit: indent two spaces, here and below -- To view, visit http://gerrit.cloudera.org:8080/11410 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I47e3ad4cbf844909bdaf22a6f9d7bd915dce3f19 Gerrit-Change-Number: 11410 Gerrit-PatchSet: 11 Gerrit-Owner: Sahil Takiar <[email protected]> Gerrit-Reviewer: David Knupp <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Michael Brown <[email protected]> Gerrit-Reviewer: Sahil Takiar <[email protected]> Gerrit-Reviewer: Thomas Marshall <[email protected]> Gerrit-Comment-Date: Wed, 17 Oct 2018 17:34:17 +0000 Gerrit-HasComments: Yes
