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 12: (10 comments) http://gerrit.cloudera.org:8080/#/c/11410/11/be/src/common/global-flags.cc File be/src/common/global-flags.cc: http://gerrit.cloudera.org:8080/#/c/11410/11/be/src/common/global-flags.cc@164 PS11, Line 164: #ifdef SLOW_BUILD > I think it's better to copy the full line twice: Done http://gerrit.cloudera.org:8080/#/c/11410/11/be/src/util/debug-util.h File be/src/util/debug-util.h: http://gerrit.cloudera.org:8080/#/c/11410/11/be/src/util/debug-util.h@105 PS11, Line 105: > Do we know the reason? Can we find out? Yeah, I spent a while trying to debug this, still haven't found out the root cause, but I dug a little deeper and traced the behavior to the AS_STRING method. The AS_STRING method uses the "stringizing" method of the C++ preprocessor to convert a preprocessor variable to a string. It seems like this method doesn't like the fact that "ADDRESS_SANITIZER" has an underscore in it. I tested it and any preprocessor variable with an underscore gets converted to the string "1" by the AS_STRING method. I don't know why it is doing this. I could not find any similar issues online. Any ideas? At first I thought that underscore was a special character for the preprocessor, but I can't find any online documentation supporting that. I tried to add some escape characters to the underscore as well, but C++ didn't like that either. The most recent patch implements a more robust fix for this issue. It first replaces all underscores in CMAKE_BUILD_TYPE with a dash (see logic in CMakeLists.txt). And then converts the dashes back to underscores in default-path-handlers.cc. The fix itself is still a bit hacky, but at least its generic to all build types with an underscore and not specific to ASAN builds. http://gerrit.cloudera.org:8080/#/c/11410/11/be/src/util/default-path-handlers.cc File be/src/util/default-path-handlers.cc: http://gerrit.cloudera.org:8080/#/c/11410/11/be/src/util/default-path-handlers.cc@245 PS11, Line 245: #ifdef NDEBUG > I think it's more readable to write the const char* ... line twice, see my 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@108 PS8, Line 108: : > Oh, right. You have to use cls.get_... Done http://gerrit.cloudera.org:8080/#/c/11410/8/tests/common/environ.py@234 PS8, Line 234: """ > I don't feel strongly, but it seems to me that there's some missing semanti Done. Changed to ImpalaTestClusterProperties. http://gerrit.cloudera.org:8080/#/c/11410/8/tests/common/environ.py@242 PS8, Line 242: > Thanks for taking the effort, I think it's much better. Done http://gerrit.cloudera.org:8080/#/c/11410/11/tests/common/environ.py File tests/common/environ.py: http://gerrit.cloudera.org:8080/#/c/11410/11/tests/common/environ.py@115 PS11, Line 115: else LinkTypes.DYNAMIC > nit: check indent Done. In general, is there a tool we use to format Python code? It seems flake8 doesn't enforce indentation style. http://gerrit.cloudera.org:8080/#/c/11410/11/tests/common/environ.py@200 PS11, Line 200: ImpalaBuildFlavors.CODE_COVERAGE_RELEASE) > nit: indent Done http://gerrit.cloudera.org:8080/#/c/11410/11/tests/common/skip.py File tests/common/skip.py: http://gerrit.cloudera.org:8080/#/c/11410/11/tests/common/skip.py@145 PS11, Line 145: clust > nit: cluster Done http://gerrit.cloudera.org:8080/#/c/11410/11/tests/webserver/test_web_pages.py File tests/webserver/test_web_pages.py: http://gerrit.cloudera.org:8080/#/c/11410/11/tests/webserver/test_web_pages.py@93 PS11, Line 93: > nit: indent two spaces, here and below 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: 12 Gerrit-Owner: Sahil Takiar <stak...@cloudera.com> Gerrit-Reviewer: David Knupp <dkn...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Michael Brown <mi...@cloudera.com> Gerrit-Reviewer: Sahil Takiar <stak...@cloudera.com> Gerrit-Reviewer: Thomas Marshall <thomasmarsh...@cmu.edu> Gerrit-Comment-Date: Thu, 18 Oct 2018 14:36:16 +0000 Gerrit-HasComments: Yes
