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 8: (15 comments) http://gerrit.cloudera.org:8080/#/c/11410/8/be/src/common/global-flags.cc File be/src/common/global-flags.cc: http://gerrit.cloudera.org:8080/#/c/11410/8/be/src/common/global-flags.cc@164 PS8, Line 164: int32 default_kudu_client_rpc_timeout_ms = 0; This should be static const to limit the scope to the current compilation unit. Use #ifdef #else #endif to make it const. http://gerrit.cloudera.org:8080/#/c/11410/6/be/src/util/debug-util.h File be/src/util/debug-util.h: http://gerrit.cloudera.org:8080/#/c/11410/6/be/src/util/debug-util.h@106 PS6, Line 106: IsNDEBUG > I named is `IsNDEBUG` because NDEBUG seems to have a special meaning in C++ As you suggested elsewhere, let's just use #ifdef NDEBUG where we call this function. http://gerrit.cloudera.org:8080/#/c/11410/6/be/src/util/debug-util.h@130 PS6, Line 130: "DYNAMIC" : : "STATIC"; > I can change this, but this is how ClangFormat is telling me to format the Ok then :) http://gerrit.cloudera.org:8080/#/c/11410/8/be/src/util/default-path-handlers.cc File be/src/util/default-path-handlers.cc: http://gerrit.cloudera.org:8080/#/c/11410/8/be/src/util/default-path-handlers.cc@247 PS8, Line 247: AddBuildFlag("CMake_Build_Type", GetCMakeBuildType(), document, &build_flags); Let's make these lowercase, too. 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@78 PS8, Line 78: remote should this be "or_local_url"? That makes it more clear that this method is supposed to work with a local minicluster http://gerrit.cloudera.org:8080/#/c/11410/8/tests/common/environ.py@106 PS8, Line 106: web_ui Can we unify the _remote_url and _remote_web_ui methods to use _remote_url only? http://gerrit.cloudera.org:8080/#/c/11410/8/tests/common/environ.py@108 PS8, Line 108: ImpaladBuildFlagsDetector This seems not needed here and elsewhere http://gerrit.cloudera.org:8080/#/c/11410/8/tests/common/environ.py@151 PS8, Line 151: LOCAL, I'd move them to the module level and call them LOCAL_BUILD and REMOTE_BUILD or similar, seems less redundant. I don't feel strongly though. http://gerrit.cloudera.org:8080/#/c/11410/8/tests/common/environ.py@176 PS8, Line 176: Return the library link type (either static or dynamic) for the Impala under test. This returns a string. Would a LinkType enum-style class make it easier to use the result? http://gerrit.cloudera.org:8080/#/c/11410/8/tests/common/environ.py@234 PS8, Line 234: def is_remote(self): Maybe this should be called is_remote_cluster? It isn't really a build property. http://gerrit.cloudera.org:8080/#/c/11410/8/tests/common/environ.py@242 PS8, Line 242: specific_build_type, library_link_type =\ "build_type, link_type" seem sufficiently verbose here. If build_type sounds too much like debug/release, maybe we should rename it to something entirely different, e.g. build_flavor? If you feel that it's too much for this change, let's just consider the shorter link_type. http://gerrit.cloudera.org:8080/#/c/11410/6/tests/webserver/test_web_pages.py File tests/webserver/test_web_pages.py: http://gerrit.cloudera.org:8080/#/c/11410/6/tests/webserver/test_web_pages.py@83 PS6, Line 83: def test_root_consistent_build_flags(self): > I have no objections to removing it. I was under the assumptions than an AS I think it's valid combination (from ASAN's perspective) and I can't think of what would be wrong with us adding such a build in the future. How about we invert the check to make sure that if assertions are on, it's not tagged "release" and if it's tagged "debug", then they must be on? http://gerrit.cloudera.org:8080/#/c/11410/8/tests/webserver/test_web_pages.py File tests/webserver/test_web_pages.py: http://gerrit.cloudera.org:8080/#/c/11410/8/tests/webserver/test_web_pages.py@63 PS8, Line 63: This looks like it could be a bit more concise by defining a dict of key -> list of allowed values. E.g. flag_values = {'is_ndebug': ['true', 'false'], ... } On the other hand, let's not re-implement too much of the logic here. http://gerrit.cloudera.org:8080/#/c/11410/8/tests/webserver/test_web_pages.py@79 PS8, Line 79: Can you add an assertion that the IMPALAD_BUILD is local? http://gerrit.cloudera.org:8080/#/c/11410/6/www/root.tmpl File www/root.tmpl: http://gerrit.cloudera.org:8080/#/c/11410/6/www/root.tmpl@36 PS6, Line 36: > The space is there so that there is a space between each flag on the Web UI And with a single space, no space ends up between the flags? In that case, can you please add a comment so that we don't remove it by accident in the future? -- 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: 8 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, 03 Oct 2018 23:43:36 +0000 Gerrit-HasComments: Yes
