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 5: (24 comments) http://gerrit.cloudera.org:8080/#/c/11410/6/CMakeLists.txt File CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/11410/6/CMakeLists.txt@34 PS6, Line 34: add_definitions(-DIMPALA_BUILD_SHARED_LIBS=${BUILD_SHARED_LIBS}) > let's move this to config.h.in Done http://gerrit.cloudera.org:8080/#/c/11410/6/CMakeLists.txt@56 PS6, Line 56: file(APPEND "${CMAKE_SOURCE_DIR}/.cmake_build_type" ${BUILD_SHARED_LIBS}) > nit: add a \n to the end of the line (if you indent the file to end in an e Done http://gerrit.cloudera.org:8080/#/c/11410/6/CMakeLists.txt@59 PS6, Line 59: add_definitions(-DIMPALA_CMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE}) > move to config.h.in? Done http://gerrit.cloudera.org:8080/#/c/11410/6/be/CMakeLists.txt File be/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/11410/6/be/CMakeLists.txt@59 PS6, Line 59: SET(CXX_COVERAGE_FLAGS "-fprofile-arcs -ftest-coverage -DCODE_COVERAGE") > config.h.in? Removed this entirely. It was only required to define SLOW_BUILD, but that logic has moved entirely into CMakeLists.txt http://gerrit.cloudera.org:8080/#/c/11410/6/be/src/exec/kudu-util.cc File be/src/exec/kudu-util.cc: http://gerrit.cloudera.org:8080/#/c/11410/6/be/src/exec/kudu-util.cc@82 PS6, Line 82: FLAGS_kudu_client_rpc_timeout_ms > I think it'd be nicer to change the default of this value for slow builds. 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@21 PS6, Line 21: #if defined(ADDRESS_SANITIZER) || defined(THREAD_SANITIZER) \ > Can we move this to common/config.h.in and then set it from cmake? That wou Done http://gerrit.cloudera.org:8080/#/c/11410/6/be/src/util/debug-util.h@106 PS6, Line 106: IsNDEBUG > nit: IsDebugBuild() I named is `IsNDEBUG` because NDEBUG seems to have a special meaning in C++ builds (controls whether asserts are triggered or not), and I wanted to Web UI to display "Is_NDEBUG=[true/false]" because thats probably easier to understand for a C++ developer. The DEBUG vs. RELEASE differentiation is handled by CMAKE_BUILD_TYPE already anyway. However, I'm no C++ expert so I'm open to changing this. http://gerrit.cloudera.org:8080/#/c/11410/6/be/src/util/debug-util.h@130 PS6, Line 130: "DYNAMIC" : : "STATIC"; > nit: I think moving these to the previous line will make it more readable I can change this, but this is how ClangFormat is telling me to format the code. http://gerrit.cloudera.org:8080/#/c/11410/5/be/src/util/debug-util.cc File be/src/util/debug-util.cc: http://gerrit.cloudera.org:8080/#/c/11410/5/be/src/util/debug-util.cc@265 PS5, Line 265: #ifdef NDEBUG > Call IsDebugBuild()? #ifdef NDEBUG is a common pattern throughout the code already, so I'm not sure replacing it with IsNDEBUG() is worth it, if anything it probably makes the code harder to understand. http://gerrit.cloudera.org:8080/#/c/11410/6/be/src/util/default-path-handlers.cc File be/src/util/default-path-handlers.cc: http://gerrit.cloudera.org:8080/#/c/11410/6/be/src/util/default-path-handlers.cc@228 PS6, Line 228: void AddBuildFlag(const std::string& build_flag_name, const std::string& build_flag_value, > I think flag_name, flag_value or even name/key, value would be sufficiently Done http://gerrit.cloudera.org:8080/#/c/11410/6/be/src/util/default-path-handlers.cc@245 PS6, Line 245: AddBuildFlag( > why not pass build_flags into AddBuildFlag and construct the temporary valu Done http://gerrit.cloudera.org:8080/#/c/11410/6/tests/common/environ.py File tests/common/environ.py: http://gerrit.cloudera.org:8080/#/c/11410/6/tests/common/environ.py@61 PS6, Line 61: tiday > typo Done http://gerrit.cloudera.org:8080/#/c/11410/6/tests/common/environ.py@93 PS6, Line 93: connect to the local impala cluster > This is not reflected in the method name nor in the comment (it should be i Done http://gerrit.cloudera.org:8080/#/c/11410/6/tests/common/environ.py@156 PS6, Line 156: the > double word Done http://gerrit.cloudera.org:8080/#/c/11410/6/tests/common/environ.py@215 PS6, Line 215: isRemote > is_remote Done http://gerrit.cloudera.org:8080/#/c/11410/6/tests/common/environ.py@249 PS6, Line 249: IMPALAD_BUILD = ImpaladLocalBuild(IMPALA_HOME) > I think we're not usually using inheritance in similar cases in our python Removed the inheritance. I took a look at using Enums, but it required adding a new Python dependency so I thought it wasn't worth it. http://gerrit.cloudera.org:8080/#/c/11410/6/tests/common/skip.py File tests/common/skip.py: http://gerrit.cloudera.org:8080/#/c/11410/6/tests/common/skip.py@145 PS6, Line 145: Tests depend > This is used to annotate a single test -> "Test depends". Feel free to fix Done 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@53 PS6, Line 53: test_root_access > On second thought, "test_get_root_url()" seems more descriptive. Done http://gerrit.cloudera.org:8080/#/c/11410/6/tests/webserver/test_web_pages.py@57 PS6, Line 57: test_root_valid_build_flags > test_get_build_flags()? To me that implies that they would need to be valid Done http://gerrit.cloudera.org:8080/#/c/11410/6/tests/webserver/test_web_pages.py@64 PS6, Line 64: assert build_flags[0]["flag_name"] == "Is_NDEBUG" > I think if you wrap the build_flags in a dict (here and elsewhere) it'll ma Done http://gerrit.cloudera.org:8080/#/c/11410/6/tests/webserver/test_web_pages.py@66 PS6, Line 66: assert build_flags[1]["flag_name"] == "CMake_Build_Type" > I think we should switch to UPPER_WITH_SPACE, lower_with_space, or CamelWit Done 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'm not sure this is worth testing. It'll fail for a release asan build, bu I have no objections to removing it. I was under the assumptions than an ASAN release build would be invalid. All this method does right now is ensure the consistency of NDEBUG with CMAKE_BUILD_TYPE. My assumption is that these should always be consistent, unless there is something wrong with the build. http://gerrit.cloudera.org:8080/#/c/11410/6/tests/webserver/test_web_pages.py@337 PS6, Line 337: "Could not find thread matching '%s'" % pattern > indent 4 spaces for line continuations 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: > nit: extra space The space is there so that there is a space between each flag on the Web UI. e.g. Is_NDEBUG=false [space]CMake_Build_Type=ADDRESS_SANITIZER [space]Library_Link_Type=STATIC -- 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: 5 Gerrit-Owner: Sahil Takiar <stak...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Sahil Takiar <stak...@cloudera.com> Gerrit-Reviewer: Thomas Marshall <thomasmarsh...@cmu.edu> Gerrit-Comment-Date: Tue, 02 Oct 2018 20:36:46 +0000 Gerrit-HasComments: Yes
