Sahil Takiar 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 10: (18 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: const static int32 default_kudu_client_rpc_timeout_ms = > This should be static const to limit the scope to the current compilation u Done 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: case ADD > As you suggested elsewhere, let's just use #ifdef NDEBUG where we call this Done http://gerrit.cloudera.org:8080/#/c/11410/6/be/src/util/debug-util.h@106 PS6, Line 106: case ADD > As you suggested elsewhere, let's just use #ifdef NDEBUG where we call this Done http://gerrit.cloudera.org:8080/#/c/11410/6/be/src/util/debug-util.h@130 PS6, Line 130: for recursive calls. : std::string Get > Ok then :) Done http://gerrit.cloudera.org:8080/#/c/11410/6/be/src/util/debug-util.h@130 PS6, Line 130: for recursive calls. : std::string Get > Ok then :) Done 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: "true" > Let's make these lowercase, too. Done 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: ibs > should this be "or_local_url"? That makes it more clear that this method is Done http://gerrit.cloudera.org:8080/#/c/11410/8/tests/common/environ.py@106 PS8, Line 106: [0].st > Can we unify the _remote_url and _remote_web_ui methods to use _remote_url I think there was a typo here, it should be get_build_flags_from_web_ui (the remote should be removed). I've fixed that. Reading through the code, I decided to remote all the 'remote_url' phrases from the method declarations. I don't think it matters if it is a remote or a local url. So I changed 'remote_url' to 'web_ui' in the method names, which I think is more descriptive. http://gerrit.cloudera.org:8080/#/c/11410/8/tests/common/environ.py@108 PS8, Line 108: : > This seems not needed here and elsewhere Yeah, but flake8 gives me a warning saying "undefined name 'validate_build_flags'", so I thought its a style requirement? http://gerrit.cloudera.org:8080/#/c/11410/8/tests/common/environ.py@151 PS8, Line 151: assert response.status_code == requests.codes.ok,\ > I'd move them to the module level and call them LOCAL_BUILD and REMOTE_BUIL Done http://gerrit.cloudera.org:8080/#/c/11410/8/tests/common/environ.py@176 PS8, Line 176: def __init__(self, build_flavor, library_link_type, local_or_remote_build): > This returns a string. Would a LinkType enum-style class make it easier to Done http://gerrit.cloudera.org:8080/#/c/11410/8/tests/common/environ.py@234 PS8, Line 234: """ > Maybe this should be called is_remote_cluster? It isn't really a build prop Changed to is_remote_cluster What about changing the class name from ImpaldBuild to ImpaladProperties? In which case a method such as is_remote_cluster would make more sense http://gerrit.cloudera.org:8080/#/c/11410/8/tests/common/environ.py@242 PS8, Line 242: > "build_type, link_type" seem sufficiently verbose here. If build_type sound Changed it to build_flavor. Required some re-factoring to other classes too. http://gerrit.cloudera.org:8080/#/c/11410/9/tests/run-tests.py File tests/run-tests.py: http://gerrit.cloudera.org:8080/#/c/11410/9/tests/run-tests.py@40 PS9, Line 40: 'webserver' > I think your approach makes a lot of sense. I'd also be comfortable with ju Agree this is a good approach, thanks Thomas! I did some more digging and it looks like the long term solution is to fix IMPALA-4417. However, that seems like a pretty big change that is out of the scope of this patch. I like the idea of having both a blacklist and a whitelist as it forces the developer to choose the type of the test folder they are adding. I think thats better than relying on a default behavior that can easily bite devs in the foot. 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: > I think it's valid combination (from ASAN's perspective) and I can't think Done 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 -> Ok, I'll just leave it as is then. http://gerrit.cloudera.org:8080/#/c/11410/8/tests/webserver/test_web_pages.py@79 PS8, Line 79: get_build_flags_from_web_ui(self.ROOT_URL.format(port)) > Can you add an assertion that the IMPALAD_BUILD is local? Done 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: > And with a single space, no space ends up between the flags? In that case, With the current diff, the the UI displays this: Build Flags: is_ndebug=false cmake_build_type=DEBUG library_link_type=STATIC Without the space it would display this: Build Flags: is_ndebug=falsecmake_build_type=DEBUGlibrary_link_type=STATIC Added the comment -- 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: 10 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: Tue, 16 Oct 2018 12:01:15 +0000 Gerrit-HasComments: Yes
