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 1: (25 comments) http://gerrit.cloudera.org:8080/#/c/11410/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11410/1//COMMIT_MSG@7 PS1, Line 7: IMPALA-6249: Expose CMAKE_BUILD_TYPE via web UI for build type detection > Update to reflect that several flags are exposed? Done http://gerrit.cloudera.org:8080/#/c/11410/1//COMMIT_MSG@10 PS1, Line 10: veiwed > typo Done http://gerrit.cloudera.org:8080/#/c/11410/1//COMMIT_MSG@14 PS1, Line 14: remotely compiled Impala repo. > It's more helpful for Impala running on remote hosts, where it was compiled Done http://gerrit.cloudera.org:8080/#/c/11410/1/be/src/util/debug-util.h File be/src/util/debug-util.h: http://gerrit.cloudera.org:8080/#/c/11410/1/be/src/util/debug-util.h@101 PS1, Line 101: std::string GetBuildType(); > It looks like CMAKE_BUILD_TYPE can take different values (CMake docs say "P Done http://gerrit.cloudera.org:8080/#/c/11410/1/be/src/util/debug-util.h@101 PS1, Line 101: std::string GetBuildType(); > Can you think of a more descriptive name here that would distinguish it fro Done http://gerrit.cloudera.org:8080/#/c/11410/1/be/src/util/debug-util.cc File be/src/util/debug-util.cc: http://gerrit.cloudera.org:8080/#/c/11410/1/be/src/util/debug-util.cc@89 PS1, Line 89: #define STRINGIFY(x) #x > You can use AS_STRING() from gutil/macros.h Done http://gerrit.cloudera.org:8080/#/c/11410/1/be/src/util/debug-util.cc@267 PS1, Line 267: ss << GetDaemonBuildVersion() > With the ifdefs gone, can you improve the line wrapping? Since all the code changes are in debug-util.h now I decided to just leave this entire file alone. http://gerrit.cloudera.org:8080/#/c/11410/1/be/src/util/debug-util.cc@277 PS1, Line 277: string GetBuildType() { > Can you make these three constexpr? In particular I'm curious whether the s Done http://gerrit.cloudera.org:8080/#/c/11410/1/be/src/util/debug-util.cc@277 PS1, Line 277: string GetBuildType() { > It doesn't look like strcmp can be called inside a constexpr. Seems C++ (at Done http://gerrit.cloudera.org:8080/#/c/11410/1/be/src/util/debug-util.cc@277 PS1, Line 277: string GetBuildType() { > something like s[0] == 'O' && s[1] == 'N' might work. Done http://gerrit.cloudera.org:8080/#/c/11410/1/be/src/util/default-path-handlers.cc File be/src/util/default-path-handlers.cc: http://gerrit.cloudera.org:8080/#/c/11410/1/be/src/util/default-path-handlers.cc@228 PS1, Line 228: Value AddBuildFlag(const std::string& build_flag_name, const std::string& build_flag_value, > line too long (91 > 90) Done http://gerrit.cloudera.org:8080/#/c/11410/1/be/src/util/default-path-handlers.cc@228 PS1, Line 228: Value AddBuildFlag(const std::string& build_flag_name, const std::string& build_flag_value, > By convention we only return primitive types. In this case you should pass Done http://gerrit.cloudera.org:8080/#/c/11410/1/be/src/util/default-path-handlers.cc@229 PS1, Line 229: Document* document) { > use spaces instead of tabs Done http://gerrit.cloudera.org:8080/#/c/11410/1/be/src/util/default-path-handlers.cc@229 PS1, Line 229: Document* document) { > tab used for whitespace Done http://gerrit.cloudera.org:8080/#/c/11410/1/be/src/util/default-path-handlers.cc@235 PS1, Line 235: return build_type; > tab used for whitespace Done http://gerrit.cloudera.org:8080/#/c/11410/1/be/src/util/default-path-handlers.cc@245 PS1, Line 245: document->GetAllocator()); > tab used for whitespace Done http://gerrit.cloudera.org:8080/#/c/11410/1/be/src/util/default-path-handlers.cc@247 PS1, Line 247: document->GetAllocator()); > tab used for whitespace Done http://gerrit.cloudera.org:8080/#/c/11410/1/be/src/util/default-path-handlers.cc@249 PS1, Line 249: document->GetAllocator()); > tab used for whitespace Done http://gerrit.cloudera.org:8080/#/c/11410/1/tests/webserver/test_web_pages.py File tests/webserver/test_web_pages.py: http://gerrit.cloudera.org:8080/#/c/11410/1/tests/webserver/test_web_pages.py@51 PS1, Line 51: test_root > If you make this test_root_access or similar, you can select it using -k wi Done http://gerrit.cloudera.org:8080/#/c/11410/1/tests/webserver/test_web_pages.py@52 PS1, Line 52: Test > nit: Tests (we tend to use declarative form instead of imperative for comme Done http://gerrit.cloudera.org:8080/#/c/11410/1/tests/webserver/test_web_pages.py@55 PS1, Line 55: def test_root_version(self): > Sure, I think its fine to revert IMPALA-6947 and do it a different way. Done http://gerrit.cloudera.org:8080/#/c/11410/1/tests/webserver/test_web_pages.py@55 PS1, Line 55: def test_root_version(self): > Our tests currently support getting the version already, but that only work Done http://gerrit.cloudera.org:8080/#/c/11410/1/tests/webserver/test_web_pages.py@61 PS1, Line 61: response_json["build_flags"] > Having a variable for response_json["build_flags"] might make things more r Done http://gerrit.cloudera.org:8080/#/c/11410/1/tests/webserver/test_web_pages.py@121 PS1, Line 121: return json.loads(responses[0].text) > assert len(responses) > 0 Done http://gerrit.cloudera.org:8080/#/c/11410/1/www/root.tmpl File www/root.tmpl: http://gerrit.cloudera.org:8080/#/c/11410/1/www/root.tmpl@22 PS1, Line 22: <h2>Impala Server Mode: {{?is_coordinator}}Coordinator{{/is_coordinator}} > while you're here you could remove the trailing whitespace Done -- 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: 1 Gerrit-Owner: Sahil Takiar <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Sahil Takiar <[email protected]> Gerrit-Reviewer: Thomas Marshall <[email protected]> Gerrit-Comment-Date: Thu, 27 Sep 2018 17:23:59 +0000 Gerrit-HasComments: Yes
