[Impala-ASF-CR] IMPALA-6249: Expose several build flags via web UI
Impala Public Jenkins 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 14: Verified+1 -- 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: 14 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Mon, 05 Nov 2018 22:47:30 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6249: Expose several build flags via web UI
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11410 ) Change subject: IMPALA-6249: Expose several build flags via web UI .. IMPALA-6249: Expose several build flags via web UI Exposes a list of build flags via the impalad web UI. The build flags can be viewed on the root page under the "Version" section. They can be accessed via other tests through the debug version of the root page (e.g. adding to the URL). The build flags are listed in a JSON array so that they can be parsed easily. This should help run Impala tests against a remote Impala cluster. The build flags are read in CMakeLists.txt and then stored in preprocessor variables. Three build flags are exposed as part of this commit: - Is_NDEBUG = [true, false] - Whether NDEBUG was true or false at compile time - CMake_Build_Type = [DEBUG, RELEASE, ADDRESS_SANITIZER, TIDY, UBSAN, UBSAN_FULL, TSAN, CODE_COVERAGE_RELEASE, CODE_COVERAGE_DEBUG] - The value of CMAKE_BUILD_TYPE at compile time - Library_Link_Type = [DYNAMIC, STATIC] - Derived from the compile time value of BUILD_SHARED_LIBS There are a few other minor changes that are apart of this commit: * The patch modifies environ.py so that it supports fetching build metadata for both local and remote clusters. * The tests under the tests/webserver directory were not being run because 'webserver' was not whitelisted in tests/run-tests.py. This patch fixes that and addresses several test failures in run-tests.py. * It reverts part of IMPALA-6947 so that their is no dependency from start-impala-cluster.py to environ.py. The timeout discussed IMPALA-6947 is now set at compile time. Testing: Added new tests to webserver/test_web_pages.py to ensure that the build flags are being set. Some tests are only run when run against a local cluster because we have no way of getting the build info from a remote cluster, whereas local clusters contain a .cmake_build_type file. Change-Id: I47e3ad4cbf844909bdaf22a6f9d7bd915dce3f19 Reviewed-on: http://gerrit.cloudera.org:8080/11410 Reviewed-by: Impala Public Jenkins Tested-by: Impala Public Jenkins --- M CMakeLists.txt M be/src/common/config.h.in M be/src/common/global-flags.cc M be/src/util/debug-util.h M be/src/util/default-path-handlers.cc M bin/start-impala-cluster.py M tests/common/environ.py M tests/common/skip.py M tests/conftest.py M tests/custom_cluster/test_admission_controller.py M tests/custom_cluster/test_automatic_invalidation.py M tests/custom_cluster/test_exchange_delays.py M tests/custom_cluster/test_metadata_replicas.py M tests/custom_cluster/test_restart_services.py M tests/query_test/test_hdfs_caching.py M tests/query_test/test_runtime_filters.py M tests/run-tests.py M tests/statestore/test_statestore.py M tests/webserver/test_web_pages.py M www/root.tmpl 20 files changed, 368 insertions(+), 96 deletions(-) Approvals: Impala Public Jenkins: Looks good to me, approved; Verified -- 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: merged Gerrit-Change-Id: I47e3ad4cbf844909bdaf22a6f9d7bd915dce3f19 Gerrit-Change-Number: 11410 Gerrit-PatchSet: 15 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Marshall
[Impala-ASF-CR] IMPALA-6249: Expose several build flags via web UI
Impala Public Jenkins 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 14: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3422/ DRY_RUN=false -- 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: 14 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Mon, 05 Nov 2018 18:38:00 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6249: Expose several build flags via web UI
Impala Public Jenkins 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 14: Code-Review+2 -- 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: 14 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Mon, 05 Nov 2018 18:37:59 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6249: Expose several build flags via web UI
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 13: Code-Review+2 Upgrading to +2 after checking with Tim and Thomas. -- 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: 13 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Mon, 05 Nov 2018 18:37:38 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6249: Expose several build flags via web UI
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 13: Code-Review+1 -- 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: 13 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Mon, 05 Nov 2018 16:37:02 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6249: Expose several build flags via web UI
Impala Public Jenkins 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 13: Verified+1 -- 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: 13 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Sat, 03 Nov 2018 01:30:45 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6249: Expose several build flags via web UI
Impala Public Jenkins 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 13: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3414/ DRY_RUN=true -- 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: 13 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Fri, 02 Nov 2018 21:38:45 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6249: Expose several build flags via web UI
David Knupp 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 13: Code-Review+1 > Patch Set 13: > > (1 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: 13 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Fri, 02 Nov 2018 21:35:00 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6249: Expose several build flags via web UI
Impala Public Jenkins 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 13: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1266/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- 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: 13 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Fri, 02 Nov 2018 20:33:14 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6249: Expose several build flags via web UI
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 13: (1 comment) http://gerrit.cloudera.org:8080/#/c/11410/12/tests/run-tests.py File tests/run-tests.py: http://gerrit.cloudera.org:8080/#/c/11410/12/tests/run-tests.py@42 PS12, Line 42: # A list of helper directories that do not contain any tests. The purpose of this > Ah yeah, I just saw the comment in JIRA after I made this comment. I unders 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: 13 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Fri, 02 Nov 2018 20:03:23 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6249: Expose several build flags via web UI
Hello Thomas Marshall, Lars Volker, Michael Brown, David Knupp, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11410 to look at the new patch set (#13). Change subject: IMPALA-6249: Expose several build flags via web UI .. IMPALA-6249: Expose several build flags via web UI Exposes a list of build flags via the impalad web UI. The build flags can be viewed on the root page under the "Version" section. They can be accessed via other tests through the debug version of the root page (e.g. adding to the URL). The build flags are listed in a JSON array so that they can be parsed easily. This should help run Impala tests against a remote Impala cluster. The build flags are read in CMakeLists.txt and then stored in preprocessor variables. Three build flags are exposed as part of this commit: - Is_NDEBUG = [true, false] - Whether NDEBUG was true or false at compile time - CMake_Build_Type = [DEBUG, RELEASE, ADDRESS_SANITIZER, TIDY, UBSAN, UBSAN_FULL, TSAN, CODE_COVERAGE_RELEASE, CODE_COVERAGE_DEBUG] - The value of CMAKE_BUILD_TYPE at compile time - Library_Link_Type = [DYNAMIC, STATIC] - Derived from the compile time value of BUILD_SHARED_LIBS There are a few other minor changes that are apart of this commit: * The patch modifies environ.py so that it supports fetching build metadata for both local and remote clusters. * The tests under the tests/webserver directory were not being run because 'webserver' was not whitelisted in tests/run-tests.py. This patch fixes that and addresses several test failures in run-tests.py. * It reverts part of IMPALA-6947 so that their is no dependency from start-impala-cluster.py to environ.py. The timeout discussed IMPALA-6947 is now set at compile time. Testing: Added new tests to webserver/test_web_pages.py to ensure that the build flags are being set. Some tests are only run when run against a local cluster because we have no way of getting the build info from a remote cluster, whereas local clusters contain a .cmake_build_type file. Change-Id: I47e3ad4cbf844909bdaf22a6f9d7bd915dce3f19 --- M CMakeLists.txt M be/src/common/config.h.in M be/src/common/global-flags.cc M be/src/util/debug-util.h M be/src/util/default-path-handlers.cc M bin/start-impala-cluster.py M tests/common/environ.py M tests/common/skip.py M tests/conftest.py M tests/custom_cluster/test_admission_controller.py M tests/custom_cluster/test_automatic_invalidation.py M tests/custom_cluster/test_exchange_delays.py M tests/custom_cluster/test_metadata_replicas.py M tests/custom_cluster/test_restart_services.py M tests/query_test/test_hdfs_caching.py M tests/query_test/test_runtime_filters.py M tests/run-tests.py M tests/statestore/test_statestore.py M tests/webserver/test_web_pages.py M www/root.tmpl 20 files changed, 368 insertions(+), 96 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/10/11410/13 -- 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: newpatchset Gerrit-Change-Id: I47e3ad4cbf844909bdaf22a6f9d7bd915dce3f19 Gerrit-Change-Number: 11410 Gerrit-PatchSet: 13 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Marshall
[Impala-ASF-CR] IMPALA-6249: Expose several build flags via web UI
David Knupp 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: (1 comment) http://gerrit.cloudera.org:8080/#/c/11410/12/tests/run-tests.py File tests/run-tests.py: http://gerrit.cloudera.org:8080/#/c/11410/12/tests/run-tests.py@42 PS12, Line 42: INVALID_TEST_DIRS = ['aux_parquet_data_load', 'test-hive-udfs', 'comparison', 'benchmark', > Yes, that correct. It prevents devs from adding a new folder under tests/ a Ah yeah, I just saw the comment in JIRA after I made this comment. I understand the necessity of doing something like this, but -- ugh, it's a little depressing. We really backed ourselves into a corner with this approach. Can you add a comment here that explains the need for this list, such that someone looking at this file later can more easily understand the rationale at a glance, outside of the context of this code review and the related JIRA? Also, perhaps a better name would be something like TEST_UTILITY_DIRS or TEST_HELPER_DIRS? Thanks! -- 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 Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 01 Nov 2018 17:54:07 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6249: Expose several build flags via web UI
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: (1 comment) http://gerrit.cloudera.org:8080/#/c/11410/12/tests/run-tests.py File tests/run-tests.py: http://gerrit.cloudera.org:8080/#/c/11410/12/tests/run-tests.py@42 PS12, Line 42: INVALID_TEST_DIRS = ['aux_parquet_data_load', 'test-hive-udfs', 'comparison', 'benchmark', > Just to make sure I understand this, the purpose of this second list is to Yes, that correct. It prevents devs from adding a new folder under tests/ and then forgetting to update the VALID_TEST_DIRS object above. In which case their new tests won't run on Jenkins. -- 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 Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 01 Nov 2018 17:31:54 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6249: Expose several build flags via web UI
David Knupp 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: (1 comment) http://gerrit.cloudera.org:8080/#/c/11410/12/tests/run-tests.py File tests/run-tests.py: http://gerrit.cloudera.org:8080/#/c/11410/12/tests/run-tests.py@42 PS12, Line 42: INVALID_TEST_DIRS = ['aux_parquet_data_load', 'test-hive-udfs', 'comparison', 'benchmark', Just to make sure I understand this, the purpose of this second list is to help prevent new tests being inadvertently skipped in the future? -- 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 Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 01 Nov 2018 17:25:31 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6249: Expose several build flags via web UI
Tim Armstrong 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: According to JIRA IMPALA-7691 is a blocker for 3.1. Do we plan to move this review along or should we change the target for IMPALA-7691? -- 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 Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 31 Oct 2018 22:36:44 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6249: Expose several build flags via web UI
Impala Public Jenkins 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: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1087/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- 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 Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Thu, 18 Oct 2018 14:59:17 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6249: Expose several build flags via web UI
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: @David, can you take a look at the changes to run-tests.py - the change is related to IMPALA-4417 which is assigned to you. The goal of the changes is to prevent things like IMPALA-7691 from occurring again. -- 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 Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Thu, 18 Oct 2018 14:38:26 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6249: Expose several build flags via web UI
Hello Thomas Marshall, Lars Volker, Michael Brown, David Knupp, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11410 to look at the new patch set (#12). Change subject: IMPALA-6249: Expose several build flags via web UI .. IMPALA-6249: Expose several build flags via web UI Exposes a list of build flags via the impalad web UI. The build flags can be viewed on the root page under the "Version" section. They can be accessed via other tests through the debug version of the root page (e.g. adding to the URL). The build flags are listed in a JSON array so that they can be parsed easily. This should help run Impala tests against a remote Impala cluster. The build flags are read in CMakeLists.txt and then stored in preprocessor variables. Three build flags are exposed as part of this commit: - Is_NDEBUG = [true, false] - Whether NDEBUG was true or false at compile time - CMake_Build_Type = [DEBUG, RELEASE, ADDRESS_SANITIZER, TIDY, UBSAN, TSAN, CODE_COVERAGE_RELEASE, CODE_COVERAGE_DEBUG] - The value of CMAKE_BUILD_TYPE at compile time - Library_Link_Type = [DYNAMIC, STATIC] - Derived from the compile time value of BUILD_SHARED_LIBS There are a few other minor changes that are apart of this commit: * The patch modifies environ.py so that it supports fetching build metadata for both local and remote clusters. * The tests under the tests/webserver directory were not being run because 'webserver' was not whitelisted in tests/run-tests.py. This patch fixes that and addresses several test failures in run-tests.py. * It reverts part of IMPALA-6947 so that their is no dependency from start-impala-cluster.py to environ.py. The timeout discussed IMPALA-6947 is now set at compile time. Testing: Added new tests to webserver/test_web_pages.py to ensure that the build flags are being set. Some tests are only run when run against a local cluster because we have no way of getting the build info from a remote cluster, whereas local clusters contain a .cmake_build_type file. Change-Id: I47e3ad4cbf844909bdaf22a6f9d7bd915dce3f19 --- M CMakeLists.txt M be/src/common/config.h.in M be/src/common/global-flags.cc M be/src/util/debug-util.h M be/src/util/default-path-handlers.cc M bin/start-impala-cluster.py M tests/common/environ.py M tests/common/skip.py M tests/conftest.py M tests/custom_cluster/test_admission_controller.py M tests/custom_cluster/test_automatic_invalidation.py M tests/custom_cluster/test_exchange_delays.py M tests/custom_cluster/test_metadata_replicas.py M tests/custom_cluster/test_restart_services.py M tests/query_test/test_hdfs_caching.py M tests/query_test/test_runtime_filters.py M tests/run-tests.py M tests/statestore/test_statestore.py M tests/webserver/test_web_pages.py M www/root.tmpl 20 files changed, 362 insertions(+), 96 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/10/11410/12 -- 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: newpatchset Gerrit-Change-Id: I47e3ad4cbf844909bdaf22a6f9d7bd915dce3f19 Gerrit-Change-Number: 11410 Gerrit-PatchSet: 12 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Marshall
[Impala-ASF-CR] IMPALA-6249: Expose several build flags via web UI
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 11: (11 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: const static int32 default_kudu_client_rpc_timeout_ms = I think it's better to copy the full line twice: #ifdef ... static const a = ... #else static const a = ... #endif also nit: ~/i1(master) ?$ git grep "static const" be | wc -l 1100 ~/i1(master) ?$ git grep "const static" be | wc -l 48 ~/i1(master) ?$ 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: For some reason Do we know the reason? Can we find out? 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: const char* is_ndebug = I think it's more readable to write the const char* ... line twice, see my comment elsewhere. 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: : > Yeah, but flake8 gives me a warning saying "undefined name 'validate_build_ Oh, right. You have to use cls.get_... http://gerrit.cloudera.org:8080/#/c/11410/8/tests/common/environ.py@234 PS8, Line 234: """ > Changed to is_remote_cluster I don't feel strongly, but it seems to me that there's some missing semantics, too. remote_cluster refers to the fact that the whole cluster under test is not available locally, and Impalad usually refers to the daemon (lots of this is historic). Maybe ImpalaTestClusterProperties or something similar would be an option, too. Please pick what you like best. http://gerrit.cloudera.org:8080/#/c/11410/8/tests/common/environ.py@242 PS8, Line 242: > Changed it to build_flavor. Required some re-factoring to other classes too Thanks for taking the effort, I think it's much better. 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 http://gerrit.cloudera.org:8080/#/c/11410/11/tests/common/environ.py@200 PS11, Line 200: ImpalaBuildFlavors.CODE_COVERAGE_RELEASE) nit: indent 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: build nit: cluster http://gerrit.cloudera.org:8080/#/c/11410/11/tests/run-tests.py File tests/run-tests.py: http://gerrit.cloudera.org:8080/#/c/11410/11/tests/run-tests.py@42 PS11, Line 42: INVALID_TEST_DIRS = ['aux_parquet_data_load', 'test-hive-udfs', 'comparison', 'benchmark', I think this is better and obviously less error-prone then the previous solution. Please check with David though, who's the assignee of IMPALA-4417. 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: assert not build_flags["cmake_build_type"] in ["release"] nit: indent two spaces, here and below -- 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: 11 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Wed, 17 Oct 2018 17:34:17 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6249: Expose several build flags via web UI
Impala Public Jenkins 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 11: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1058/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- 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: 11 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Tue, 16 Oct 2018 13:27:17 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6249: Expose several build flags via web UI
Hello Thomas Marshall, Lars Volker, Michael Brown, David Knupp, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11410 to look at the new patch set (#11). Change subject: IMPALA-6249: Expose several build flags via web UI .. IMPALA-6249: Expose several build flags via web UI Exposes a list of build flags via the impalad web UI. The build flags can be viewed on the root page under the "Version" section. They can be accessed via other tests through the debug version of the root page (e.g. adding to the URL). The build flags are listed in a JSON array so that they can be parsed easily. This should help run Impala tests against a remote Impala cluster. The build flags are read in CMakeLists.txt and then stored in preprocessor variables. Three build flags are exposed as part of this commit: - Is_NDEBUG = [true, false] - Whether NDEBUG was true or false at compile time - CMake_Build_Type = [DEBUG, RELEASE, ADDRESS_SANITIZER, TIDY, UBSAN, TSAN, CODE_COVERAGE_RELEASE, CODE_COVERAGE_DEBUG] - The value of CMAKE_BUILD_TYPE at compile time - Library_Link_Type = [DYNAMIC, STATIC] - Derived from the compile time value of BUILD_SHARED_LIBS There are a few other minor changes that are apart of this commit: * The patch modifies environ.py so that it supports fetching build metadata for both local and remote clusters. * The tests under the tests/webserver directory were not being run because 'webserver' was not whitelisted in tests/run-tests.py. This patch fixes that and addresses several test failures in run-tests.py. * It reverts part of IMPALA-6947 so that their is no dependency from start-impala-cluster.py to environ.py. The timeout discussed IMPALA-6947 is now set at compile time. Testing: Added new tests to webserver/test_web_pages.py to ensure that the build flags are being set. Some tests are only run when run against a local cluster because we have no way of getting the build info from a remote cluster, whereas local clusters contain a .cmake_build_type file. Change-Id: I47e3ad4cbf844909bdaf22a6f9d7bd915dce3f19 --- M CMakeLists.txt M be/src/common/config.h.in M be/src/common/global-flags.cc M be/src/util/debug-util.h M be/src/util/default-path-handlers.cc M bin/start-impala-cluster.py M tests/common/environ.py M tests/common/skip.py M tests/conftest.py M tests/custom_cluster/test_admission_controller.py M tests/custom_cluster/test_exchange_delays.py M tests/custom_cluster/test_metadata_replicas.py M tests/custom_cluster/test_restart_services.py M tests/query_test/test_hdfs_caching.py M tests/query_test/test_runtime_filters.py M tests/run-tests.py M tests/statestore/test_statestore.py M tests/webserver/test_web_pages.py M www/root.tmpl 19 files changed, 356 insertions(+), 87 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/10/11410/11 -- 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: newpatchset Gerrit-Change-Id: I47e3ad4cbf844909bdaf22a6f9d7bd915dce3f19 Gerrit-Change-Number: 11410 Gerrit-PatchSet: 11 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Marshall
[Impala-ASF-CR] IMPALA-6249: Expose several build flags via web UI
Impala Public Jenkins 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: (2 comments) http://gerrit.cloudera.org:8080/#/c/11410/10/tests/custom_cluster/test_metadata_replicas.py File tests/custom_cluster/test_metadata_replicas.py: http://gerrit.cloudera.org:8080/#/c/11410/10/tests/custom_cluster/test_metadata_replicas.py@21 PS10, Line 21: from tests.common.environ import build_flavor_timeout flake8: F401 'tests.common.environ.build_flavor_timeout' imported but unused http://gerrit.cloudera.org:8080/#/c/11410/10/tests/run-tests.py File tests/run-tests.py: http://gerrit.cloudera.org:8080/#/c/11410/10/tests/run-tests.py@213 PS10, Line 213: i flake8: E501 line too long (92 > 90 characters) -- 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 Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Tue, 16 Oct 2018 12:26:20 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6249: Expose several build flags via web UI
Impala Public Jenkins 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: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1057/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- 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 Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Tue, 16 Oct 2018 12:21:25 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6249: Expose several build flags via web UI
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:
[Impala-ASF-CR] IMPALA-6249: Expose several build flags via web UI
Hello Thomas Marshall, Lars Volker, Michael Brown, David Knupp, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11410 to look at the new patch set (#10). Change subject: IMPALA-6249: Expose several build flags via web UI .. IMPALA-6249: Expose several build flags via web UI Exposes a list of build flags via the impalad web UI. The build flags can be viewed on the root page under the "Version" section. They can be accessed via other tests through the debug version of the root page (e.g. adding to the URL). The build flags are listed in a JSON array so that they can be parsed easily. This should help run Impala tests against a remote Impala cluster. The build flags are read in CMakeLists.txt and then stored in preprocessor variables. Three build flags are exposed as part of this commit: - Is_NDEBUG = [true, false] - Whether NDEBUG was true or false at compile time - CMake_Build_Type = [DEBUG, RELEASE, ADDRESS_SANITIZER, TIDY, UBSAN, TSAN, CODE_COVERAGE_RELEASE, CODE_COVERAGE_DEBUG] - The value of CMAKE_BUILD_TYPE at compile time - Library_Link_Type = [DYNAMIC, STATIC] - Derived from the compile time value of BUILD_SHARED_LIBS There are a few other minor changes that are apart of this commit: * The patch modifies environ.py so that it supports fetching build metadata for both local and remote clusters. * The tests under the tests/webserver directory were not being run because 'webserver' was not whitelisted in tests/run-tests.py. This patch fixes that and addresses several test failures in run-tests.py. * It reverts part of IMPALA-6947 so that their is no dependency from start-impala-cluster.py to environ.py. The timeout discussed IMPALA-6947 is now set at compile time. Testing: Added new tests to webserver/test_web_pages.py to ensure that the build flags are being set. Some tests are only run when run against a local cluster because we have no way of getting the build info from a remote cluster, whereas local clusters contain a .cmake_build_type file. Change-Id: I47e3ad4cbf844909bdaf22a6f9d7bd915dce3f19 --- M CMakeLists.txt M be/src/common/config.h.in M be/src/common/global-flags.cc M be/src/util/debug-util.h M be/src/util/default-path-handlers.cc M bin/start-impala-cluster.py M tests/common/environ.py M tests/common/skip.py M tests/conftest.py M tests/custom_cluster/test_admission_controller.py M tests/custom_cluster/test_exchange_delays.py M tests/custom_cluster/test_metadata_replicas.py M tests/custom_cluster/test_restart_services.py M tests/query_test/test_hdfs_caching.py M tests/query_test/test_runtime_filters.py M tests/run-tests.py M tests/statestore/test_statestore.py M tests/webserver/test_web_pages.py M www/root.tmpl 19 files changed, 356 insertions(+), 87 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/10/11410/10 -- 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: newpatchset Gerrit-Change-Id: I47e3ad4cbf844909bdaf22a6f9d7bd915dce3f19 Gerrit-Change-Number: 11410 Gerrit-PatchSet: 10 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Marshall
[Impala-ASF-CR] IMPALA-6249: Expose several build flags via web UI
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 9: (1 comment) 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' > what do you think about doing something to prevent these sorts of errors in I think your approach makes a lot of sense. I'd also be comfortable with just having a blacklist. It looks like we already expected that adding a directory would include it in the execution, and if people get bitten by it, they can add it here. Alternatively we could add some magic DONT-TEST-THIS file in each folder that shouldn't get executed. I don't feel strongly and am good with either approach. -- 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: 9 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Wed, 10 Oct 2018 22:02:53 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6249: Expose several build flags via web UI
Thomas Marshall 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 9: (1 comment) 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' what do you think about doing something to prevent these sorts of errors in the future, eg: https://github.com/twmarshall/impala/commit/f5b6937a01d6a0b495a1ddb49e268f9bc08db689 (I had noticed this problem, filed IMPALA-7691, and started to fix it before realizing you already had a fix out) -- 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: 9 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Wed, 10 Oct 2018 20:44:05 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6249: Expose several build flags via web UI
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 8:
(15 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: int32 default_kudu_client_rpc_timeout_ms = 0;
This should be static const to limit the scope to the current compilation unit.
Use #ifdef #else #endif to make it const.
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: IsNDEBUG
> I named is `IsNDEBUG` because NDEBUG seems to have a special meaning in C++
As you suggested elsewhere, let's just use #ifdef NDEBUG where we call this
function.
http://gerrit.cloudera.org:8080/#/c/11410/6/be/src/util/debug-util.h@130
PS6, Line 130: "DYNAMIC" :
: "STATIC";
> I can change this, but this is how ClangFormat is telling me to format the
Ok then :)
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: AddBuildFlag("CMake_Build_Type", GetCMakeBuildType(),
document, _flags);
Let's make these lowercase, too.
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: remote
should this be "or_local_url"? That makes it more clear that this method is
supposed to work with a local minicluster
http://gerrit.cloudera.org:8080/#/c/11410/8/tests/common/environ.py@106
PS8, Line 106: web_ui
Can we unify the _remote_url and _remote_web_ui methods to use _remote_url only?
http://gerrit.cloudera.org:8080/#/c/11410/8/tests/common/environ.py@108
PS8, Line 108: ImpaladBuildFlagsDetector
This seems not needed here and elsewhere
http://gerrit.cloudera.org:8080/#/c/11410/8/tests/common/environ.py@151
PS8, Line 151: LOCAL,
I'd move them to the module level and call them LOCAL_BUILD and REMOTE_BUILD or
similar, seems less redundant. I don't feel strongly though.
http://gerrit.cloudera.org:8080/#/c/11410/8/tests/common/environ.py@176
PS8, Line 176: Return the library link type (either static or dynamic) for
the Impala under test.
This returns a string. Would a LinkType enum-style class make it easier to use
the result?
http://gerrit.cloudera.org:8080/#/c/11410/8/tests/common/environ.py@234
PS8, Line 234: def is_remote(self):
Maybe this should be called is_remote_cluster? It isn't really a build property.
http://gerrit.cloudera.org:8080/#/c/11410/8/tests/common/environ.py@242
PS8, Line 242: specific_build_type, library_link_type =\
"build_type, link_type" seem sufficiently verbose here. If build_type sounds
too much like debug/release, maybe we should rename it to something entirely
different, e.g. build_flavor? If you feel that it's too much for this change,
let's just consider the shorter link_type.
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: def test_root_consistent_build_flags(self):
> I have no objections to removing it. I was under the assumptions than an AS
I think it's valid combination (from ASAN's perspective) and I can't think of
what would be wrong with us adding such a build in the future. How about we
invert the check to make sure that if assertions are on, it's not tagged
"release" and if it's tagged "debug", then they must be on?
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 ->
list of allowed values. E.g.
flag_values = {'is_ndebug': ['true', 'false'],
... }
On the other hand, let's not re-implement too much of the logic here.
http://gerrit.cloudera.org:8080/#/c/11410/8/tests/webserver/test_web_pages.py@79
PS8, Line 79:
Can you add an assertion that the IMPALAD_BUILD is local?
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:
> The space is there so that there is a space between each flag on the Web UI
And with a single space, no space ends up between the flags? In that case, can
you please add a comment so that we don't remove it by accident in the future?
--
To view, visit
[Impala-ASF-CR] IMPALA-6249: Expose several build flags via web UI
Impala Public Jenkins 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 8: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/913/ : Initial code review checks failed. See linked job for details on the failure. -- 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: 8 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Wed, 03 Oct 2018 10:16:00 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6249: Expose several build flags via web UI
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 6: (1 comment) 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@249 PS6, Line 249: IMPALAD_BUILD = ImpaladLocalBuild(IMPALA_HOME) > I don't think we should introduce an external dependency (i.e., pip install Thanks for the tip Michael. I went ahead and used the same approach in query.py -- 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 Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Wed, 03 Oct 2018 00:16:43 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6249: Expose several build flags via web UI
Hello Thomas Marshall, Lars Volker, Michael Brown, David Knupp, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11410 to look at the new patch set (#8). Change subject: IMPALA-6249: Expose several build flags via web UI .. IMPALA-6249: Expose several build flags via web UI Exposes a list of build flags via the impalad web UI. The build flags can be viewed on the root page under the "Version" section. They can be accessed via other tests through the debug version of the root page (e.g. adding to the URL). The build flags are listed in a JSON array so that they can be parsed easily. This should help run Impala tests against a remote Impala cluster. The build flags are read in CMakeLists.txt and then stored in preprocessor variables. Three build flags are exposed as part of this commit: - Is_NDEBUG = [true, false] - Whether NDEBUG was true or false at compile time - CMake_Build_Type = [DEBUG, RELEASE, ADDRESS_SANITIZER, TIDY, UBSAN, TSAN, CODE_COVERAGE_RELEASE, CODE_COVERAGE_DEBUG] - The value of CMAKE_BUILD_TYPE at compile time - Library_Link_Type = [DYNAMIC, STATIC] - Derived from the compile time value of BUILD_SHARED_LIBS There are a few other minor changes that are apart of this commit: * The patch modifies environ.py so that it supports fetching build metadata for both local and remote clusters. * The tests under the tests/webserver directory were not being run because 'webserver' was not whitelisted in tests/run-tests.py. This patch fixes that and addresses several test failures in run-tests.py. * It reverts part of IMPALA-6947 so that their is no dependency from start-impala-cluster.py to environ.py. The timeout discussed IMPALA-6947 is now set at compile time. Testing: Added new tests to webserver/test_web_pages.py to ensure that the build flags are being set. Some tests are only run when run against a local cluster because we have no way of getting the build info from a remote cluster, whereas local clusters contain a .cmake_build_type file. Change-Id: I47e3ad4cbf844909bdaf22a6f9d7bd915dce3f19 --- M CMakeLists.txt M be/src/common/config.h.in M be/src/common/global-flags.cc M be/src/util/debug-util.h M be/src/util/default-path-handlers.cc M tests/common/environ.py M tests/common/skip.py M tests/run-tests.py M tests/webserver/test_web_pages.py M www/root.tmpl 10 files changed, 289 insertions(+), 47 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/10/11410/8 -- 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: newpatchset Gerrit-Change-Id: I47e3ad4cbf844909bdaf22a6f9d7bd915dce3f19 Gerrit-Change-Number: 11410 Gerrit-PatchSet: 8 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Marshall
[Impala-ASF-CR] IMPALA-6249: Expose several build flags via web UI
Michael Brown 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: (1 comment) 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@249 PS6, Line 249: IMPALAD_BUILD = ImpaladLocalBuild(IMPALA_HOME) > @Michael, David do we support using Enums in our Python code in Impala? Or I don't think we should introduce an external dependency (i.e., pip install something) for this. Typical usage to get enums in Python is like: A, B, C = range(3) Example: https://github.com/apache/impala/blob/ea2809f5ddcf48f3f41dcd12743e8a17b4ea8cd7/tests/comparison/query.py#L45 In that example, the items were put in as class attributes in order to provide a clear namespace. -- 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 Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Tue, 02 Oct 2018 22:08:50 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6249: Expose several build flags via web UI
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 6: (1 comment) 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@249 PS6, Line 249: IMPALAD_BUILD = ImpaladLocalBuild(IMPALA_HOME) > Removed the inheritance. I took a look at using Enums, but it required addi @Michael, David do we support using Enums in our Python code in Impala? Or something like Enums? -- 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 Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Tue, 02 Oct 2018 21:54:02 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6249: Expose several build flags via web UI
Impala Public Jenkins 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 7: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/904/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- 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: 7 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Tue, 02 Oct 2018 21:05:50 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6249: Expose several build flags via web UI
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
[Impala-ASF-CR] IMPALA-6249: Expose several build flags via web UI
Sahil Takiar has uploaded a new patch set (#7). ( http://gerrit.cloudera.org:8080/11410 ) Change subject: IMPALA-6249: Expose several build flags via web UI .. IMPALA-6249: Expose several build flags via web UI Exposes a list of build flags via the impalad web UI. The build flags can be viewed on the root page under the "Version" section. They can be accessed via other tests through the debug version of the root page (e.g. adding to the URL). The build flags are listed in a JSON array so that they can be parsed easily. This should help run Impala tests against a remote Impala cluster. The build flags are read in CMakeLists.txt and then stored in preprocessor variables. Three build flags are exposed as part of this commit: - Is_NDEBUG = [true, false] - Whether NDEBUG was true or false at compile time - CMake_Build_Type = [DEBUG, RELEASE, ADDRESS_SANITIZER, TIDY, UBSAN, TSAN, CODE_COVERAGE_RELEASE, CODE_COVERAGE_DEBUG] - The value of CMAKE_BUILD_TYPE at compile time - Library_Link_Type = [DYNAMIC, STATIC] - Derived from the compile time value of BUILD_SHARED_LIBS There are a few other minor changes that are apart of this commit: * The patch modifies environ.py so that it supports fetching build metadata for both local and remote clusters. * The tests under the tests/webserver directory were not being run because 'webserver' was not whitelisted in tests/run-tests.py. This patch fixes that and addresses several test failures in run-tests.py. * It reverts part of IMPALA-6947 so that their is no dependency from start-impala-cluster.py to environ.py. The timeout discussed IMPALA-6947 is now set at compile time. Testing: Added new tests to webserver/test_web_pages.py to ensure that the build flags are being set. Some tests are only run when run against a local cluster because we have no way of getting the build info from a remote cluster, whereas local clusters contain a .cmake_build_type file. Change-Id: I47e3ad4cbf844909bdaf22a6f9d7bd915dce3f19 --- M CMakeLists.txt M be/src/common/config.h.in M be/src/common/global-flags.cc M be/src/util/debug-util.h M be/src/util/default-path-handlers.cc M tests/common/environ.py M tests/common/skip.py M tests/run-tests.py M tests/webserver/test_web_pages.py M www/root.tmpl 10 files changed, 278 insertions(+), 47 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/10/11410/7 -- 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: newpatchset Gerrit-Change-Id: I47e3ad4cbf844909bdaf22a6f9d7bd915dce3f19 Gerrit-Change-Number: 11410 Gerrit-PatchSet: 7 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Marshall
[Impala-ASF-CR] IMPALA-6249: Expose several build flags via web UI
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
[Impala-ASF-CR] IMPALA-6249: Expose several build flags via web UI
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
[Impala-ASF-CR] IMPALA-6249: Expose several build flags via web UI
Impala Public Jenkins 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: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/832/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- 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 Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Thu, 27 Sep 2018 01:26:21 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6249: Expose several build flags via web UI
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: > (1 comment) Added SLOW_BUILD to util/debug-util.h -- 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 Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Thu, 27 Sep 2018 00:56:49 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6249: Expose several build flags via web UI
Hello Thomas Marshall, Lars Volker, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11410 to look at the new patch set (#5). Change subject: IMPALA-6249: Expose several build flags via web UI .. IMPALA-6249: Expose several build flags via web UI Exposes a list of build flags via the impalad web UI. The build flags can be viewed on the root page under the "Version" section. They can be accessed via other tests through the debug version of the root page (e.g. adding to the URL). The build flags are listed in a JSON array so that they can be parsed easily. This should help run Impala tests against a remote Impala cluster. The build flags are read in CMakeLists.txt and then stored in preprocessor variables. Three build flags are exposed as part of this commit: - Is_NDEBUG = [true, false] - Whether NDEBUG was true or false at compile time - CMake_Build_Type = [DEBUG, RELEASE, ADDRESS_SANITIZER, TIDY, UBSAN, TSAN, CODE_COVERAGE_RELEASE, CODE_COVERAGE_DEBUG] - The value of CMAKE_BUILD_TYPE at compile time - Library_Link_Type = [DYNAMIC, STATIC] - Derived from the compile time value of BUILD_SHARED_LIBS There are a few other minor changes that are apart of this commit: * The patch modifies environ.py so that it supports fetching build metadata for both local and remote clusters. * The tests under the tests/webserver directory were not being run because 'webserver' was not whitelisted in tests/run-tests.py. This patch fixes that and addresses several test failures in run-tests.py. * It reverts part of IMPALA-6947 so that their is no dependency from start-impala-cluster.py to environ.py. The timeout discussed IMPALA-6947 is now set at compile time. Testing: Added new tests to webserver/test_web_pages.py to ensure that the build flags are being set. Some tests are only run when run against a local cluster because we have no way of getting the build info from a remote cluster, whereas local clusters contain a .cmake_build_type file. Change-Id: I47e3ad4cbf844909bdaf22a6f9d7bd915dce3f19 --- M CMakeLists.txt M be/CMakeLists.txt M be/src/exec/kudu-util.cc M be/src/util/debug-util.h M be/src/util/default-path-handlers.cc M tests/common/environ.py M tests/common/skip.py M tests/run-tests.py M tests/webserver/test_web_pages.py M www/root.tmpl 10 files changed, 282 insertions(+), 42 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/10/11410/5 -- 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: newpatchset Gerrit-Change-Id: I47e3ad4cbf844909bdaf22a6f9d7bd915dce3f19 Gerrit-Change-Number: 11410 Gerrit-PatchSet: 5 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Marshall
[Impala-ASF-CR] IMPALA-6249: Expose several build flags via web UI
Thomas Marshall 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 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/11410/4/be/src/exec/kudu-util.cc File be/src/exec/kudu-util.cc: http://gerrit.cloudera.org:8080/#/c/11410/4/be/src/exec/kudu-util.cc@79 PS4, Line 79: defined(ADDRESS_SANITIZER) || defined(THREAD_SANITIZER) \ : || defined(UNDEFINED_SANITIZER) || defined(CODE_COVERAGE) Did you consider making this a general SLOW_BUILD flag defined in util/? -- 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: 4 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Wed, 26 Sep 2018 19:11:02 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6249: Expose several build flags via web UI
Impala Public Jenkins 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 4: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/815/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- 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: 4 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Wed, 26 Sep 2018 17:37:20 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6249: Expose several build flags via web UI
Hello Thomas Marshall, Lars Volker, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11410 to look at the new patch set (#4). Change subject: IMPALA-6249: Expose several build flags via web UI .. IMPALA-6249: Expose several build flags via web UI Exposes a list of build flags via the impalad web UI. The build flags can be viewed on the root page under the "Version" section. They can be accessed via other tests through the debug version of the root page (e.g. adding to the URL). The build flags are listed in a JSON array so that they can be parsed easily. This should help run Impala tests against a remote Impala cluster. The build flags are read in CMakeLists.txt and then stored in preprocessor variables. Three build flags are exposed as part of this commit: - Is_NDEBUG = [true, false] - Whether NDEBUG was true or false at compile time - CMake_Build_Type = [DEBUG, RELEASE, ADDRESS_SANITIZER, TIDY, UBSAN, TSAN, CODE_COVERAGE_RELEASE, CODE_COVERAGE_DEBUG] - The value of CMAKE_BUILD_TYPE at compile time - Library_Link_Type = [DYNAMIC, STATIC] - Derived from the compile time value of BUILD_SHARED_LIBS There are a few other minor changes that are apart of this commit: * The patch modifies environ.py so that it supports fetching build metadata for both local and remote clusters. * The tests under the tests/webserver directory were not being run because 'webserver' was not whitelisted in tests/run-tests.py. This patch fixes that and addresses several test failures in run-tests.py. * It reverts part of IMPALA-6947 so that their is no dependency from start-impala-cluster.py to environ.py. The timeout discussed IMPALA-6947 is now set at compile time. Testing: Added new tests to webserver/test_web_pages.py to ensure that the build flags are being set. Some tests are only run when run against a local cluster because we have no way of getting the build info from a remote cluster, whereas local clusters contain a .cmake_build_type file. Change-Id: I47e3ad4cbf844909bdaf22a6f9d7bd915dce3f19 --- M CMakeLists.txt M be/CMakeLists.txt M be/src/exec/kudu-util.cc M be/src/util/debug-util.h M be/src/util/default-path-handlers.cc M tests/common/environ.py M tests/common/skip.py M tests/run-tests.py M tests/webserver/test_web_pages.py M www/root.tmpl 10 files changed, 277 insertions(+), 42 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/10/11410/4 -- 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: newpatchset Gerrit-Change-Id: I47e3ad4cbf844909bdaf22a6f9d7bd915dce3f19 Gerrit-Change-Number: 11410 Gerrit-PatchSet: 4 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Marshall
[Impala-ASF-CR] IMPALA-6249: Expose several build flags via web UI
Impala Public Jenkins 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 3: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/805/ : Initial code review checks failed. See linked job for details on the failure. -- 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: 3 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Wed, 26 Sep 2018 01:55:14 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6249: Expose several build flags via web UI
Impala Public Jenkins 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 2: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/804/ : Initial code review checks failed. See linked job for details on the failure. -- 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: 2 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Wed, 26 Sep 2018 01:47:31 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6249: Expose several build flags via web UI
Hello Thomas Marshall, Lars Volker, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11410 to look at the new patch set (#3). Change subject: IMPALA-6249: Expose several build flags via web UI .. IMPALA-6249: Expose several build flags via web UI Exposes a list of build flags via the impalad web UI. The build flags can be viewed on the root page under the "Version" section. They can be accessed via other tests through the debug version of the root page (e.g. adding to the URL). The build flags are listed in a JSON array so that they can be parsed easily. This should help run Impala tests against a remote Impala cluster. The build flags are read in CMakeLists.txt and then stored in preprocessor variables. Three build flags are exposed as part of this commit: - Is_NDEBUG = [true, false] - Whether NDEBUG was true or false at compile time - CMake_Build_Type = [DEBUG, RELEASE, ADDRESS_SANITIZER, TIDY, UBSAN, TSAN, CODE_COVERAGE_RELEASE, CODE_COVERAGE_DEBUG] - The value of CMAKE_BUILD_TYPE at compile time - Library_Link_Type = [DYNAMIC, STATIC] - Derived from the compile time value of BUILD_SHARED_LIBS There are a few other minor changes that are apart of this commit: * The patch modifies environ.py so that it supports fetching build metadata for both local and remote clusters. * The tests under the tests/webserver directory were not being run because 'webserver' was not whitelisted in tests/run-tests.py. This patch fixes that and addresses several test failures in run-tests.py. * It reverts part of IMPALA-6947 so that their is no dependency from start-impala-cluster.py to environ.py. The timeout discussed IMPALA-6947 is now set at compile time. Testing: Added new tests to webserver/test_web_pages.py to ensure that the build flags are being set. Some tests are only run when run against a local cluster because we have no way of getting the build info from a remote cluster, whereas local clusters contain a .cmake_build_type file. Change-Id: I47e3ad4cbf844909bdaf22a6f9d7bd915dce3f19 --- M CMakeLists.txt M be/CMakeLists.txt M be/src/exec/kudu-util.cc M be/src/util/debug-util.h M be/src/util/default-path-handlers.cc M tests/common/environ.py M tests/common/skip.py M tests/run-tests.py M tests/webserver/test_web_pages.py M www/root.tmpl 10 files changed, 277 insertions(+), 42 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/10/11410/3 -- 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: newpatchset Gerrit-Change-Id: I47e3ad4cbf844909bdaf22a6f9d7bd915dce3f19 Gerrit-Change-Number: 11410 Gerrit-PatchSet: 3 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Marshall
[Impala-ASF-CR] IMPALA-6249: Expose several build flags via web UI
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: (2 comments) 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 I decided to keep this flag and to rename it to IsNDEBUG because thats really what its doing. I tried adding some static_asserts, but the Impala-lzo code wouldn't compile. I'm not entirely sure why, but my guess is that since Impala-lzo cmake doesn't contain the same build flags as Impala, there are issues during compilation. So instead I added a Python test to validate that the flags are consistent. 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@55 PS1, Line 55: def test_root_version(self): > Sure, I think its fine to revert IMPALA-6947 and do it a different way. Reverted the portion of IMPALA-6947 specific to -kudu_client_rpc_timeout_ms, instead the timeout is set at compile time if the build type is asan, tsan, ubsan, or has code coverage. I modified environ.py so that is supports fetching build flags from a remote cluster. If the env variable IMPALA_REMOTE_URL is set then environ.py will use the specified URL to fetch the build flags, otherwise it falls back to its old behavior. Looking through the tests, seems a lot of them are still hardcoded to connect to localhost (e.g. the websever) tests. However, this change is at least a start. -- 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 Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Wed, 26 Sep 2018 01:19:23 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6249: Expose several build flags via web UI
Impala Public Jenkins 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 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/11410/2/tests/common/environ.py File tests/common/environ.py: http://gerrit.cloudera.org:8080/#/c/11410/2/tests/common/environ.py@247 PS2, Line 247: if not IMPALA_REMOTE_URL: flake8: E305 expected 2 blank lines after class or function definition, found 1 -- 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: 2 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Wed, 26 Sep 2018 01:12:36 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6249: Expose several build flags via web UI
Hello Thomas Marshall, Lars Volker, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11410 to look at the new patch set (#2). Change subject: IMPALA-6249: Expose several build flags via web UI .. IMPALA-6249: Expose several build flags via web UI Exposes a list of build flags via the impalad web UI. The build flags can be viewed on the root page under the "Version" section. They can be accessed via other tests through the debug version of the root page (e.g. adding to the URL). The build flags are listed in a JSON array so that they can be parsed easily. This should help run Impala tests against a remote Impala cluster. The build flags are read in CMakeLists.txt and then stored in preprocessor variables. Three build flags are exposed as part of this commit: - Is_NDEBUG = [true, false] - Whether NDEBUG was true or false at compile time - CMake_Build_Type = [DEBUG, RELEASE, ADDRESS_SANITIZER, TIDY, UBSAN, TSAN, CODE_COVERAGE_RELEASE, CODE_COVERAGE_DEBUG] - The value of CMAKE_BUILD_TYPE at compile time - Library_Link_Type = [DYNAMIC, STATIC] - Derived from the compile time value of BUILD_SHARED_LIBS There are a few other minor changes that are apart of this commit: * The patch modifies environ.py so that it supports fetching build metadata for both local and remote clusters. * The tests under the tests/webserver directory were not being run because 'webserver' was not whitelisted in tests/run-tests.py. This patch fixes that and addresses several test failures in run-tests.py. * It reverts part of IMPALA-6947 so that their is no dependency from start-impala-cluster.py to environ.py. The timeout discussed IMPALA-6947 is now set at compile time. Testing: Added new tests to webserver/test_web_pages.py to ensure that the build flags are being set. Some tests are only run when run against a local cluster because we have no way of getting the build info from a remote cluster, whereas local clusters contain a .cmake_build_type file. Change-Id: I47e3ad4cbf844909bdaf22a6f9d7bd915dce3f19 --- M CMakeLists.txt M be/CMakeLists.txt M be/src/exec/kudu-util.cc M be/src/util/debug-util.h M be/src/util/default-path-handlers.cc M tests/common/environ.py M tests/common/skip.py M tests/run-tests.py M tests/webserver/test_web_pages.py M www/root.tmpl 10 files changed, 276 insertions(+), 42 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/10/11410/2 -- 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: newpatchset Gerrit-Change-Id: I47e3ad4cbf844909bdaf22a6f9d7bd915dce3f19 Gerrit-Change-Number: 11410 Gerrit-PatchSet: 2 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Thomas Marshall
