[Impala-ASF-CR] IMPALA-6249: Expose several build flags via web UI

["Lars Volker (Code Review)" <ge...@cloudera.org>]

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 6:

(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


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 empty line), or move the \n to the front of the second line (if you don't intend the file to end in an empty line. I'd prefer the former.


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?


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?


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. That way, -help will also reflect the change, and it stays consistent across all places in the code that use the flag.


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 would reflect that it's cmake that decides whether the build is slow, not the preprocessor.


http://gerrit.cloudera.org:8080/#/c/11410/6/be/src/util/debug-util.h@106
PS6, Line 106: IsNDEBUG
nit: IsDebugBuild()


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


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()?


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 descriptive.


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 value there?


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


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 in both).


http://gerrit.cloudera.org:8080/#/c/11410/6/tests/common/environ.py@156
PS6, Line 156: the
double word


http://gerrit.cloudera.org:8080/#/c/11410/6/tests/common/environ.py@215
PS6, Line 215: isRemote
is_remote


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 codebase and I'm concerned that this will make the code harder to read for others. To simplify it, I think we could pass specific_build_type, library_link_type, and is_remote into the ctor of ImpaladBuild, and then instantiate it with either

  ImpaladBuild(ImpaladBuildFlagsDetector.detect_using_remote_url(impala_remote_url), True)

or

  ImpaladBuild(ImpaladBuildFlagsDetector.detect_using_build_root(impala_build_root), False)

We could further improve readability with by passing constants LOCAL=0 and REMOTE=1 to the ctor. I don't feel strongly about this, please check with David or Mike who have a more consistent view on our Python style.


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 in the line above while you're here.


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.


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.


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 make the tests independent of the flag order, and much easier to read and write. Something like

  flags = dict((f['flag_name'], f['flag_value']) for f in build_flags)

should work. You can check len(flags) == len(build_flags) to make sure there were no collisions, and you should probably use better names. :)


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 CamelWithoutSpace, but mixing all three of them seems too much to me.


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, but I don't think there's anything inherently wrong with such a build. Should we remove the test?


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


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



-- 
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: 6
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Comment-Date: Mon, 01 Oct 2018 20:28:00 +0000
Gerrit-HasComments: Yes