[Impala-ASF-CR] IMPALA-9865: part 2/2: add verbosity to profile tool
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/16881 ) Change subject: IMPALA-9865: part 2/2: add verbosity to profile tool .. Patch Set 8: Code-Review+1 (2 comments) Thanks for the explanation. LGTM. http://gerrit.cloudera.org:8080/#/c/16881/7/be/src/util/runtime-profile.cc File be/src/util/runtime-profile.cc: http://gerrit.cloudera.org:8080/#/c/16881/7/be/src/util/runtime-profile.cc@2723 PS7, Line 2723: // Legacy profile did not show per-instance stats for averaged profile. > Done Resolved http://gerrit.cloudera.org:8080/#/c/16881/6/tests/observability/test_profile_tool.py File tests/observability/test_profile_tool.py: http://gerrit.cloudera.org:8080/#/c/16881/6/tests/observability/test_profile_tool.py@36 PS6, Line 36: self._compare_profile_tool_output([], > Yeah, producing identical output is not necessarily the goal of this tool, Got it, thanks! -- To view, visit http://gerrit.cloudera.org:8080/16881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I82618a813e29af7996dfaed78873b2a73bc0231d Gerrit-Change-Number: 16881 Gerrit-PatchSet: 8 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 22 Dec 2020 06:58:34 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9865: part 2/2: add verbosity to profile tool
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16881 ) Change subject: IMPALA-9865: part 2/2: add verbosity to profile tool .. Patch Set 8: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/7895/ : 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/16881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I82618a813e29af7996dfaed78873b2a73bc0231d Gerrit-Change-Number: 16881 Gerrit-PatchSet: 8 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 22 Dec 2020 06:54:07 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9865: part 2/2: add verbosity to profile tool
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16881 ) Change subject: IMPALA-9865: part 2/2: add verbosity to profile tool .. Patch Set 6: (2 comments) http://gerrit.cloudera.org:8080/#/c/16881/7/be/src/util/runtime-profile.cc File be/src/util/runtime-profile.cc: http://gerrit.cloudera.org:8080/#/c/16881/7/be/src/util/runtime-profile.cc@2723 PS7, Line 2723: if (verbosity <= Verbosity::LEGACY) continue; > Looks like this condition can be flipped and merged with the branch bellow? Done http://gerrit.cloudera.org:8080/#/c/16881/6/tests/observability/test_profile_tool.py File tests/observability/test_profile_tool.py: http://gerrit.cloudera.org:8080/#/c/16881/6/tests/observability/test_profile_tool.py@36 PS6, Line 36: self._compare_profile_tool_output([], > Ok, I understand the difference now. Setting --profile_verbosity=legacy mea Yeah, producing identical output is not necessarily the goal of this tool, but we do want to be able to parse older profiles and we want to have some tests for pretty-printing so that it's rendered as expected. One thing I'm hoping to achieve with the tool is to buy more flexibility around the text representation of the V2 profiles. We had a lot of dilemmas about how much information to include, but those are less important if we can print the profile with more/less information later on; or if we can improve the output in the tool independent of the version of Impala profiles being analyzed. -- To view, visit http://gerrit.cloudera.org:8080/16881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I82618a813e29af7996dfaed78873b2a73bc0231d Gerrit-Change-Number: 16881 Gerrit-PatchSet: 6 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 22 Dec 2020 06:33:10 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9865: part 2/2: add verbosity to profile tool
Hello Riza Suminto, Joe McDonnell, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16881 to look at the new patch set (#8). Change subject: IMPALA-9865: part 2/2: add verbosity to profile tool .. IMPALA-9865: part 2/2: add verbosity to profile tool Adds a --profile_verbosity option for impala-profile-tool with the following levels: * 0: minimal * 1: legacy - matches old output, this is the default still * 2: default - basic descriptive stats, used for V2 profile. * 3: extended * 4: full This will help with transition to the V2 profile because we can have a nice, high-level, readable text profile by default with the option to produce more detailed profiles and alternate views of the profile from the thrift profile. Use the profile version in impala-profile-tool to dump the more verbose output for the V2 profile while preserving the same output for the legacy profile. Reduce verbosity of v2 profile output - only include mean/min/max by default. I intend to refine the output at the different verbosity levels for the v2 profiles further as part of IMPALA-9382, it is still fairly noisy. Fix output with/without gen_experimental_profile - there was a small difference in that the summary stats were not output in the averaged profile. Testing: * Add an end-to-end test that generates output for a small profile log and compares against expected files. * Tweak other profile tests to reflect changes to output. Change-Id: I82618a813e29af7996dfaed78873b2a73bc0231d --- M be/src/util/impala-profile-tool.cc M be/src/util/runtime-profile-counters.h M be/src/util/runtime-profile-test.cc M be/src/util/runtime-profile.cc M be/src/util/runtime-profile.h M bin/jenkins/dockerized-impala-run-tests.sh M bin/rat_exclude_files.txt A testdata/impala-profiles/README A testdata/impala-profiles/impala_profile_log_tpcds_compute_stats A testdata/impala-profiles/impala_profile_log_tpcds_compute_stats.expected.json A testdata/impala-profiles/impala_profile_log_tpcds_compute_stats.expected.pretty.json A testdata/impala-profiles/impala_profile_log_tpcds_compute_stats.expected.pretty_extended.json A testdata/impala-profiles/impala_profile_log_tpcds_compute_stats.expected.txt A testdata/impala-profiles/impala_profile_log_tpcds_compute_stats_default.expected.txt A testdata/impala-profiles/impala_profile_log_tpcds_compute_stats_extended.expected.txt M testdata/workloads/tpch/queries/runtime-profile-aggregated.test A tests/observability/test_profile_tool.py M tests/query_test/test_scanners.py 18 files changed, 95,019 insertions(+), 125 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/81/16881/8 -- To view, visit http://gerrit.cloudera.org:8080/16881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I82618a813e29af7996dfaed78873b2a73bc0231d Gerrit-Change-Number: 16881 Gerrit-PatchSet: 8 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-10317: Add query option that limits huge joins at runtime
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/16706 ) Change subject: IMPALA-10317: Add query option that limits huge joins at runtime .. IMPALA-10317: Add query option that limits huge joins at runtime This patch adds support for limiting the rows produced by a join node such that runaway join queries can be prevented. The limit is specified by a query option. Queries exceeding that limit get terminated. The checking runs periodically, so the actual rows produced may go somewhat over the limit. JOIN_ROWS_PRODUCED_LIMIT is exposed as an advanced query option. Rows produced Query profile is updated to include query wide and per backend metrics for RowsReturned. Example from " set JOIN_ROWS_PRODUCED_LIMIT = 1000; select count(*) from tpch_parquet.lineitem l1 cross join (select * from tpch_parquet.lineitem l2 limit 5) l3;": NESTED_LOOP_JOIN_NODE (id=2): - InactiveTotalTime: 107.534ms - PeakMemoryUsage: 16.00 KB (16384) - ProbeRows: 1.02K (1024) - ProbeTime: 0.000ns - RowsReturned: 10.00M (10002025) - RowsReturnedRate: 749.58 K/sec - TotalTime: 13s337ms Testing: Added tests for JOIN_ROWS_PRODUCED_LIMIT Change-Id: Idbca7e053b61b4e31b066edcfb3b0398fa859d02 Reviewed-on: http://gerrit.cloudera.org:8080/16706 Reviewed-by: Impala Public Jenkins Tested-by: Impala Public Jenkins --- M be/src/runtime/coordinator-backend-state.cc M be/src/runtime/coordinator.h M be/src/runtime/fragment-instance-state.cc M be/src/runtime/fragment-instance-state.h M be/src/runtime/query-state.cc M be/src/service/impala-server.cc M be/src/service/query-options-test.cc M be/src/service/query-options.cc M be/src/service/query-options.h M common/protobuf/control_service.proto M common/thrift/ImpalaInternalService.thrift M common/thrift/ImpalaService.thrift M common/thrift/generate_error_codes.py M testdata/workloads/functional-query/queries/QueryTest/query-resource-limits.test 14 files changed, 167 insertions(+), 5 deletions(-) Approvals: Impala Public Jenkins: Looks good to me, approved; Verified -- To view, visit http://gerrit.cloudera.org:8080/16706 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Idbca7e053b61b4e31b066edcfb3b0398fa859d02 Gerrit-Change-Number: 16706 Gerrit-PatchSet: 8 Gerrit-Owner: Fucun Chu Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Fucun Chu Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-10317: Add query option that limits huge joins at runtime
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16706 ) Change subject: IMPALA-10317: Add query option that limits huge joins at runtime .. Patch Set 7: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/16706 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idbca7e053b61b4e31b066edcfb3b0398fa859d02 Gerrit-Change-Number: 16706 Gerrit-PatchSet: 7 Gerrit-Owner: Fucun Chu Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Fucun Chu Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Tue, 22 Dec 2020 06:10:38 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10374: Limit iteration at BufferedTupleStream::DebugString
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16884 ) Change subject: IMPALA-10374: Limit iteration at BufferedTupleStream::DebugString .. Patch Set 4: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/16884 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6626c8d54f35f303c01f85be1dd9aa54c8ad9a2d Gerrit-Change-Number: 16884 Gerrit-PatchSet: 4 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Tue, 22 Dec 2020 04:58:40 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10374: Limit iteration at BufferedTupleStream::DebugString
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16884 ) Change subject: IMPALA-10374: Limit iteration at BufferedTupleStream::DebugString .. Patch Set 4: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/6800/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/16884 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6626c8d54f35f303c01f85be1dd9aa54c8ad9a2d Gerrit-Change-Number: 16884 Gerrit-PatchSet: 4 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Tue, 22 Dec 2020 04:58:41 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10374: Limit iteration at BufferedTupleStream::DebugString
Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/16884 ) Change subject: IMPALA-10374: Limit iteration at BufferedTupleStream::DebugString .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/16884 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6626c8d54f35f303c01f85be1dd9aa54c8ad9a2d Gerrit-Change-Number: 16884 Gerrit-PatchSet: 3 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Tue, 22 Dec 2020 04:57:46 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9966: Add missing breaks in SetQueryOption
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16892 ) Change subject: IMPALA-9966: Add missing breaks in SetQueryOption .. Patch Set 2: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/16892 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I696204c7036c0059fe4eed2d23c8e947b6c53d8d Gerrit-Change-Number: 16892 Gerrit-PatchSet: 2 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Tue, 22 Dec 2020 04:31:55 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9966: Add missing breaks in SetQueryOption
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/16892 ) Change subject: IMPALA-9966: Add missing breaks in SetQueryOption .. IMPALA-9966: Add missing breaks in SetQueryOption Change-Id: I696204c7036c0059fe4eed2d23c8e947b6c53d8d Reviewed-on: http://gerrit.cloudera.org:8080/16892 Reviewed-by: Impala Public Jenkins Tested-by: Impala Public Jenkins --- M be/src/service/query-options.cc 1 file changed, 7 insertions(+), 5 deletions(-) Approvals: Impala Public Jenkins: Looks good to me, approved; Verified -- To view, visit http://gerrit.cloudera.org:8080/16892 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I696204c7036c0059fe4eed2d23c8e947b6c53d8d Gerrit-Change-Number: 16892 Gerrit-PatchSet: 3 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-10374: Limit iteration at BufferedTupleStream::DebugString
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16884 ) Change subject: IMPALA-10374: Limit iteration at BufferedTupleStream::DebugString .. Patch Set 3: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/7894/ : 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/16884 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6626c8d54f35f303c01f85be1dd9aa54c8ad9a2d Gerrit-Change-Number: 16884 Gerrit-PatchSet: 3 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Tue, 22 Dec 2020 03:41:28 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10374: Limit iteration at BufferedTupleStream::DebugString
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/16884 ) Change subject: IMPALA-10374: Limit iteration at BufferedTupleStream::DebugString .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/16884/2/be/src/runtime/buffered-tuple-stream.h File be/src/runtime/buffered-tuple-stream.h: http://gerrit.cloudera.org:8080/#/c/16884/2/be/src/runtime/buffered-tuple-stream.h@563 PS2, Line 563: friend class SimpleTupleStreamTest_ShortDebugString_Test; : : /// Runtime state instance used to check for cancellation. Not own > Yeah, please delete them. Done -- To view, visit http://gerrit.cloudera.org:8080/16884 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6626c8d54f35f303c01f85be1dd9aa54c8ad9a2d Gerrit-Change-Number: 16884 Gerrit-PatchSet: 3 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Tue, 22 Dec 2020 03:26:51 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10374: Limit iteration at BufferedTupleStream::DebugString
Hello Quanlong Huang, Csaba Ringhofer, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16884 to look at the new patch set (#3). Change subject: IMPALA-10374: Limit iteration at BufferedTupleStream::DebugString .. IMPALA-10374: Limit iteration at BufferedTupleStream::DebugString BufferedTupleStream::DebugString() iterate std::list that can potentially grow very large. As consequent, the returned string can grow large as well and cause a problem as previously happen in IMPALA-9851. With this patch, BufferedTupleStream::DebugString() only include maximum of 100 first pages of page list. Testing: - Add new be test SimpleTupleStreamTest.ShortDebugString in buffered-tuple-stream-test.cc - Pass core tests Change-Id: I6626c8d54f35f303c01f85be1dd9aa54c8ad9a2d --- M be/src/runtime/buffered-tuple-stream-test.cc M be/src/runtime/buffered-tuple-stream.cc M be/src/runtime/buffered-tuple-stream.h 3 files changed, 73 insertions(+), 7 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/84/16884/3 -- To view, visit http://gerrit.cloudera.org:8080/16884 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I6626c8d54f35f303c01f85be1dd9aa54c8ad9a2d Gerrit-Change-Number: 16884 Gerrit-PatchSet: 3 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang Gerrit-Reviewer: Riza Suminto
[Impala-ASF-CR] IMPALA-9865: part 2/2: add verbosity to profile tool
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/16881 ) Change subject: IMPALA-9865: part 2/2: add verbosity to profile tool .. Patch Set 7: (6 comments) I have 1 follow up question and 1 new finding. http://gerrit.cloudera.org:8080/#/c/16881/6/be/src/util/impala-profile-tool.cc File be/src/util/impala-profile-tool.cc: http://gerrit.cloudera.org:8080/#/c/16881/6/be/src/util/impala-profile-tool.cc@88 PS6, Line 88: RuntimeProfileBase::Verbosity configured_verbosity = > We shouldn't actually be using the default value, since we only use if it's Done http://gerrit.cloudera.org:8080/#/c/16881/6/be/src/util/impala-profile-tool.cc@136 PS6, Line 136: if (FLAGS_profile_verbosity == "") { > Yes and no. We can set the verbosity level to any level for any profile, bu Make sense. Thanks for clarifying this. http://gerrit.cloudera.org:8080/#/c/16881/7/be/src/util/runtime-profile.cc File be/src/util/runtime-profile.cc: http://gerrit.cloudera.org:8080/#/c/16881/7/be/src/util/runtime-profile.cc@2723 PS7, Line 2723: if (verbosity <= Verbosity::LEGACY) continue; Looks like this condition can be flipped and merged with the branch bellow? if (verbosity >= Verbosity::DEFAULT && v.second.second.size() > 1) { http://gerrit.cloudera.org:8080/#/c/16881/6/bin/rat_exclude_files.txt File bin/rat_exclude_files.txt: http://gerrit.cloudera.org:8080/#/c/16881/6/bin/rat_exclude_files.txt@157 PS6, Line 157: testdata/impala-profiles/impala_profile_log_tpcds_compute_stats.expected.txt > Done Resolved http://gerrit.cloudera.org:8080/#/c/16881/6/tests/observability/test_profile_tool.py File tests/observability/test_profile_tool.py: http://gerrit.cloudera.org:8080/#/c/16881/6/tests/observability/test_profile_tool.py@36 PS6, Line 36: self._compare_profile_tool_output([], > Leaving --profile_verbosity unset is different from setting it to --legacy Ok, I understand the difference now. Setting --profile_verbosity=legacy means forcing legacy format, regardless of the profile_version. Since we don't test with --profile_verbosity=legacy, am I correct to assume that accurate backward-compatible parsing of v2 profile to v1 representation is not the goal of impala-profile-tool? http://gerrit.cloudera.org:8080/#/c/16881/6/tests/observability/test_profile_tool.py@38 PS6, Line 38: get_profile_path('impala_profile_log_tpcds_compute_stats.expected.txt')) > See above comment - this is actually kinda different from legacy Got it, thanks! -- To view, visit http://gerrit.cloudera.org:8080/16881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I82618a813e29af7996dfaed78873b2a73bc0231d Gerrit-Change-Number: 16881 Gerrit-PatchSet: 7 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 22 Dec 2020 03:12:09 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10374: Limit iteration at BufferedTupleStream::DebugString
Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/16884 ) Change subject: IMPALA-10374: Limit iteration at BufferedTupleStream::DebugString .. Patch Set 2: Code-Review+2 (1 comment) LGTM. Thanks for handling this! http://gerrit.cloudera.org:8080/#/c/16884/2/be/src/runtime/buffered-tuple-stream.h File be/src/runtime/buffered-tuple-stream.h: http://gerrit.cloudera.org:8080/#/c/16884/2/be/src/runtime/buffered-tuple-stream.h@563 PS2, Line 563: friend class ArrayTupleStreamTest_TestArrayDeepCopy_Test; : friend class ArrayTupleStreamTest_TestComputeRowSize_Test; : friend class MultiNullableTupleStreamTest_TestComputeRowSize_Test; > I notice that since IMPALA-10337 changed BufferedTupleStream::ComputeRowSiz Yeah, please delete them. -- To view, visit http://gerrit.cloudera.org:8080/16884 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6626c8d54f35f303c01f85be1dd9aa54c8ad9a2d Gerrit-Change-Number: 16884 Gerrit-PatchSet: 2 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Tue, 22 Dec 2020 02:31:48 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9865: part 2/2: add verbosity to profile tool
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16881 ) Change subject: IMPALA-9865: part 2/2: add verbosity to profile tool .. Patch Set 7: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/7893/ : 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/16881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I82618a813e29af7996dfaed78873b2a73bc0231d Gerrit-Change-Number: 16881 Gerrit-PatchSet: 7 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 22 Dec 2020 01:38:11 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9865: part 2/2: add verbosity to profile tool
Hello Riza Suminto, Joe McDonnell, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16881 to look at the new patch set (#7). Change subject: IMPALA-9865: part 2/2: add verbosity to profile tool .. IMPALA-9865: part 2/2: add verbosity to profile tool Adds a --profile_verbosity option for impala-profile-tool with the following levels: * 0: minimal * 1: legacy - matches old output, this is the default still * 2: default - basic descriptive stats, used for V2 profile. * 3: extended * 4: full Use the profile version in impala-profile-tool to dump the more verbose output for the V2 profile while preserving the same output for the legacy profile. Reduce verbosity of v2 profile output - only include mean/min/max by default. I intend to refine the output at the different verbosity levels for the v2 profiles further as part of IMPALA-9382, it is still fairly noisy. Fix output with/without gen_experimental_profile - there was a small difference in that the summary stats were not output in the averaged profile. Testing: * Add an end-to-end test that generates output for a small profile log and compares against expected files. * Tweak other profile tests to reflect changes to output. Change-Id: I82618a813e29af7996dfaed78873b2a73bc0231d --- M be/src/util/impala-profile-tool.cc M be/src/util/runtime-profile-counters.h M be/src/util/runtime-profile-test.cc M be/src/util/runtime-profile.cc M be/src/util/runtime-profile.h M bin/jenkins/dockerized-impala-run-tests.sh M bin/rat_exclude_files.txt A testdata/impala-profiles/README A testdata/impala-profiles/impala_profile_log_tpcds_compute_stats A testdata/impala-profiles/impala_profile_log_tpcds_compute_stats.expected.json A testdata/impala-profiles/impala_profile_log_tpcds_compute_stats.expected.pretty.json A testdata/impala-profiles/impala_profile_log_tpcds_compute_stats.expected.pretty_extended.json A testdata/impala-profiles/impala_profile_log_tpcds_compute_stats.expected.txt A testdata/impala-profiles/impala_profile_log_tpcds_compute_stats_default.expected.txt A testdata/impala-profiles/impala_profile_log_tpcds_compute_stats_extended.expected.txt M testdata/workloads/tpch/queries/runtime-profile-aggregated.test A tests/observability/test_profile_tool.py M tests/query_test/test_scanners.py 18 files changed, 95,019 insertions(+), 124 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/81/16881/7 -- To view, visit http://gerrit.cloudera.org:8080/16881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I82618a813e29af7996dfaed78873b2a73bc0231d Gerrit-Change-Number: 16881 Gerrit-PatchSet: 7 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-9865: part 2/2: add verbosity to profile tool
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16881 ) Change subject: IMPALA-9865: part 2/2: add verbosity to profile tool .. Patch Set 6: (5 comments) http://gerrit.cloudera.org:8080/#/c/16881/6/be/src/util/impala-profile-tool.cc File be/src/util/impala-profile-tool.cc: http://gerrit.cloudera.org:8080/#/c/16881/6/be/src/util/impala-profile-tool.cc@88 PS6, Line 88: RuntimeProfileBase::Verbosity configured_verbosity; > Is this meant to be initialized as RuntimeProfileBase::Verbosity::LEGACY ? We shouldn't actually be using the default value, since we only use if it's non-empty and parsing succeeds. But clang-tidy was complaining about this anyway so fixing it :) Compiling /home/ubuntu/Impala/be/src/util/impala-profile-tool.cc:134:47: warning: variable 'configured_verbosity' may be uninitialized when used here [clang-diagnostic-conditional-uninitialized] RuntimeProfileBase::Verbosity verbosity = configured_verbosity; ^ /home/ubuntu/Impala/be/src/util/impala-profile-tool.cc:88:3: note: variable 'configured_verbosity' is declared here RuntimeProfileBase::Verbosity configured_verbosity; ^ http://gerrit.cloudera.org:8080/#/c/16881/6/be/src/util/impala-profile-tool.cc@136 PS6, Line 136: verbosity = profile_version < 2 ? RuntimeProfileBase::Verbosity::LEGACY : > In case of v1 profile input and --profile_verbosity=default, this line does Yes and no. We can set the verbosity level to any level for any profile, but it only emits what is present in the thrift profile. Older v1 profiles won't have the TAggregatedRuntimeProfileNode set for the averaged profile, so it will only instantiate a regular RuntimeProfile object when parsing the thrift, which means it will only print the legacy profile information. http://gerrit.cloudera.org:8080/#/c/16881/6/bin/rat_exclude_files.txt File bin/rat_exclude_files.txt: http://gerrit.cloudera.org:8080/#/c/16881/6/bin/rat_exclude_files.txt@157 PS6, Line 157: testdata/impala-profiles/impala_profile_log_tpcds_compute_stats.expected > Should we add `.txt` suffix for expected output in text format? Done http://gerrit.cloudera.org:8080/#/c/16881/6/tests/observability/test_profile_tool.py File tests/observability/test_profile_tool.py: http://gerrit.cloudera.org:8080/#/c/16881/6/tests/observability/test_profile_tool.py@36 PS6, Line 36: self._compare_profile_tool_output([], > Since default verbosity might change in the future, can we add a test using Leaving --profile_verbosity unset is different from setting it to --legacy since it will auto-detect based on the file contents. I think it's a good thing if the test fails when we change the behaviour. I'd like to add some V2 profile tests when I finish up IMPALA-9382 and the format is final. http://gerrit.cloudera.org:8080/#/c/16881/6/tests/observability/test_profile_tool.py@38 PS6, Line 38: get_profile_path('impala_profile_log_tpcds_compute_stats.expected')) > Rename expected file name to "impala_profile_log_tpcds_compute_stats_legacy See above comment - this is actually kinda different from legacy -- To view, visit http://gerrit.cloudera.org:8080/16881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I82618a813e29af7996dfaed78873b2a73bc0231d Gerrit-Change-Number: 16881 Gerrit-PatchSet: 6 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 22 Dec 2020 01:15:54 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10336: Coordinator return incorrect error to client
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/16849 ) Change subject: IMPALA-10336: Coordinator return incorrect error to client .. IMPALA-10336: Coordinator return incorrect error to client Due to race condition, coordinator could set execution status as RPC aborted due to cancellation. This internal error should not be returned to client. This patch fixed the issue by setting the backend status as CANCELLED instead of ABORTED if the exec RPC was aborted due to cancellation. Testing: - Manual tests Since this is a racy bug, I could only reproduce the situation by adding some artificial delays in 3 places: QueryExecMgr.StartQuery(), Coordinator.UpdateBackendExecStatus(), and Coordinator::StartBackendExec() when running test case test_scanners.py::TestOrc::test_type_conversions_hive3. Verified that the issue did not happen after applying this patch by running test_scanners.py::TestOrc::test_type_conversions_hive3 in a loop for hours. - Passed exhausive test. Change-Id: I75f252e43006c6ff6980800e3254672de396b318 Reviewed-on: http://gerrit.cloudera.org:8080/16849 Reviewed-by: Impala Public Jenkins Tested-by: Impala Public Jenkins --- M be/src/runtime/coordinator-backend-state.cc M be/src/runtime/coordinator-backend-state.h 2 files changed, 15 insertions(+), 2 deletions(-) Approvals: Impala Public Jenkins: Looks good to me, approved; Verified -- To view, visit http://gerrit.cloudera.org:8080/16849 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I75f252e43006c6ff6980800e3254672de396b318 Gerrit-Change-Number: 16849 Gerrit-PatchSet: 5 Gerrit-Owner: Wenzhe Zhou Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Wenzhe Zhou
[Impala-ASF-CR] IMPALA-10336: Coordinator return incorrect error to client
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16849 ) Change subject: IMPALA-10336: Coordinator return incorrect error to client .. Patch Set 4: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/16849 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I75f252e43006c6ff6980800e3254672de396b318 Gerrit-Change-Number: 16849 Gerrit-PatchSet: 4 Gerrit-Owner: Wenzhe Zhou Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Wenzhe Zhou Gerrit-Comment-Date: Tue, 22 Dec 2020 01:02:46 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9975 (part 1): Various refactors for admission control daemon
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16890 ) Change subject: IMPALA-9975 (part 1): Various refactors for admission control daemon .. Patch Set 1: Code-Review+2 Thanks, this is an improvement, the duplication was unnecessary. -- To view, visit http://gerrit.cloudera.org:8080/16890 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2e097e20458354f78bfc3477cac6fb3a2835f094 Gerrit-Change-Number: 16890 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Wenzhe Zhou Gerrit-Comment-Date: Tue, 22 Dec 2020 00:52:42 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10317: Add query option that limits huge joins at runtime
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16706 ) Change subject: IMPALA-10317: Add query option that limits huge joins at runtime .. Patch Set 7: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/16706 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idbca7e053b61b4e31b066edcfb3b0398fa859d02 Gerrit-Change-Number: 16706 Gerrit-PatchSet: 7 Gerrit-Owner: Fucun Chu Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Fucun Chu Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Tue, 22 Dec 2020 00:31:24 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10317: Add query option that limits huge joins at runtime
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16706 ) Change subject: IMPALA-10317: Add query option that limits huge joins at runtime .. Patch Set 7: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/6799/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/16706 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idbca7e053b61b4e31b066edcfb3b0398fa859d02 Gerrit-Change-Number: 16706 Gerrit-PatchSet: 7 Gerrit-Owner: Fucun Chu Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Fucun Chu Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Tue, 22 Dec 2020 00:31:25 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10317: Add query option that limits huge joins at runtime
Aman Sinha has posted comments on this change. ( http://gerrit.cloudera.org:8080/16706 ) Change subject: IMPALA-10317: Add query option that limits huge joins at runtime .. Patch Set 6: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/16706 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idbca7e053b61b4e31b066edcfb3b0398fa859d02 Gerrit-Change-Number: 16706 Gerrit-PatchSet: 6 Gerrit-Owner: Fucun Chu Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Fucun Chu Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Tue, 22 Dec 2020 00:27:59 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6671: Skip locked tables from topic updates
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/16549 ) Change subject: IMPALA-6671: Skip locked tables from topic updates .. IMPALA-6671: Skip locked tables from topic updates This change adds a mechanism for topic-update thread to skip a table which is locked for more than a configurable interval from the topic updates. This is especially useful in scenarios where long running operations on a locked table (refresh, recover partitions, compute stats) block the topic update thread. This causes unrelated queries which are waiting on metadata via topic updates (catalog-v1 mode) to unnecessarily block. The ideal solution of this problem would be to make HdfsTable immutable so that there is no need for table lock. But that is large change and not easily portable to older releases of Impala. It would be taken up as a separate patch. This change introduces 2 new configurations for catalogd: 1. topic_update_tbl_max_wait_time_ms: This defines the maximum time in msecs the topic update thread waits on a locked table before skipping the table from that iteration of topic updates. The default value is 500. If this configuration is set to 0 the lock with timeout for topic update thread is disabled. 2. catalog_max_lock_skipped_topic_updates: This defines the maximum number of distinct lock operations which are skipped by topic update thread due to lock contention. Once this limit is reached, topic update thread will block until it acquires the table lock and adds it to the updates. Testing: 1. Added a test case which introduces a simulated delay in a few potentially long running statements. This causes the table to be locked for a long time. The topic update thread skips that table from updates and unrelated queries are unblocked since they receive the required metadata from updates. 2. Added a test where multiple threads run blocking statements in a loop to stress the table lock. It makes sure that topic update thread is not starved and eventually blocks on table lock by hitting the limit defined by catalog_max_lock_skipped_topic_updates. 3. Ran exhaustive tests with default configurations. Change-Id: Ic657b96edbcdc94c6b906e7ca59291f4e4715655 Reviewed-on: http://gerrit.cloudera.org:8080/16549 Reviewed-by: Impala Public Jenkins Tested-by: Impala Public Jenkins --- M be/src/catalog/catalog-server.cc M be/src/util/backend-gflag-util.cc M common/thrift/BackendGflags.thrift M fe/src/main/java/org/apache/impala/analysis/CopyTestCaseStmt.java M fe/src/main/java/org/apache/impala/catalog/Catalog.java M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java M fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java M fe/src/main/java/org/apache/impala/catalog/Table.java M fe/src/main/java/org/apache/impala/catalog/TopicUpdateLog.java M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java M fe/src/test/java/org/apache/impala/catalog/CatalogObjectToFromThriftTest.java A tests/custom_cluster/test_topic_update_frequency.py 13 files changed, 698 insertions(+), 149 deletions(-) Approvals: Impala Public Jenkins: Looks good to me, approved; Verified -- To view, visit http://gerrit.cloudera.org:8080/16549 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Ic657b96edbcdc94c6b906e7ca59291f4e4715655 Gerrit-Change-Number: 16549 Gerrit-PatchSet: 18 Gerrit-Owner: Vihang Karajgaonkar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang Gerrit-Reviewer: Shant Hovsepian Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vihang Karajgaonkar
[Impala-ASF-CR] IMPALA-6671: Skip locked tables from topic updates
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16549 ) Change subject: IMPALA-6671: Skip locked tables from topic updates .. Patch Set 17: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/16549 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic657b96edbcdc94c6b906e7ca59291f4e4715655 Gerrit-Change-Number: 16549 Gerrit-PatchSet: 17 Gerrit-Owner: Vihang Karajgaonkar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang Gerrit-Reviewer: Shant Hovsepian Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Tue, 22 Dec 2020 00:19:31 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9966: Add missing breaks in SetQueryOption
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16892 ) Change subject: IMPALA-9966: Add missing breaks in SetQueryOption .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/16892 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I696204c7036c0059fe4eed2d23c8e947b6c53d8d Gerrit-Change-Number: 16892 Gerrit-PatchSet: 2 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Mon, 21 Dec 2020 22:50:11 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9966: Add missing breaks in SetQueryOption
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16892 ) Change subject: IMPALA-9966: Add missing breaks in SetQueryOption .. Patch Set 2: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/6798/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/16892 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I696204c7036c0059fe4eed2d23c8e947b6c53d8d Gerrit-Change-Number: 16892 Gerrit-PatchSet: 2 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Mon, 21 Dec 2020 22:50:12 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9966: Add missing breaks in SetQueryOption
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/16892 ) Change subject: IMPALA-9966: Add missing breaks in SetQueryOption .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/16892 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I696204c7036c0059fe4eed2d23c8e947b6c53d8d Gerrit-Change-Number: 16892 Gerrit-PatchSet: 1 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Mon, 21 Dec 2020 22:49:17 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10325: Parquet scan should use min/max statistics to skip pages based on equi-join predicate
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16720 ) Change subject: IMPALA-10325: Parquet scan should use min/max statistics to skip pages based on equi-join predicate .. Patch Set 38: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/7892/ : 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/16720 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I379405ee75b14929df7d6b5d20dabc6f51375691 Gerrit-Change-Number: 16720 Gerrit-PatchSet: 38 Gerrit-Owner: Qifan Chen Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Mon, 21 Dec 2020 22:31:41 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10374: Limit iteration at BufferedTupleStream::DebugString
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/16884 ) Change subject: IMPALA-10374: Limit iteration at BufferedTupleStream::DebugString .. Patch Set 2: (5 comments) http://gerrit.cloudera.org:8080/#/c/16884/1/be/src/runtime/buffered-tuple-stream-test.cc File be/src/runtime/buffered-tuple-stream-test.cc: http://gerrit.cloudera.org:8080/#/c/16884/1/be/src/runtime/buffered-tuple-stream-test.cc@1472 PS1, Line 1472: SimpleTupleStreamTest > nit: To be consistent with other tests, I think the label should be "Simple Done http://gerrit.cloudera.org:8080/#/c/16884/1/be/src/runtime/buffered-tuple-stream-test.cc@1485 PS1, Line 1485: bool b = stream.AddRow(batch->GetRow(j), ); > I think we can remove this TODO. Done http://gerrit.cloudera.org:8080/#/c/16884/1/be/src/runtime/buffered-tuple-stream-test.cc@1508 PS1, Line 1508: > Can we calculate 148 out based on the vars? I'm concerning it will introduc It is not obvious for me to calculate it based on the vars. So instead, I change it to look up stream.num_pages_ directly and add verification that num_pages_ > MAX_PAGE_ITER_DEBUG, given the workload. http://gerrit.cloudera.org:8080/#/c/16884/2/be/src/runtime/buffered-tuple-stream.h File be/src/runtime/buffered-tuple-stream.h: http://gerrit.cloudera.org:8080/#/c/16884/2/be/src/runtime/buffered-tuple-stream.h@563 PS2, Line 563: friend class ArrayTupleStreamTest_TestArrayDeepCopy_Test; : friend class ArrayTupleStreamTest_TestComputeRowSize_Test; : friend class MultiNullableTupleStreamTest_TestComputeRowSize_Test; I notice that since IMPALA-10337 changed BufferedTupleStream::ComputeRowSize() from private to public, these lines are not required anymore. Can I delete them along with this patch? http://gerrit.cloudera.org:8080/#/c/16884/1/be/src/runtime/buffered-tuple-stream.cc File be/src/runtime/buffered-tuple-stream.cc: http://gerrit.cloudera.org:8080/#/c/16884/1/be/src/runtime/buffered-tuple-stream.cc@189 PS1, Line 189: \ > Comparing to the original codes, we should have "\n" here. Done. Thanks for catching this! -- To view, visit http://gerrit.cloudera.org:8080/16884 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6626c8d54f35f303c01f85be1dd9aa54c8ad9a2d Gerrit-Change-Number: 16884 Gerrit-PatchSet: 2 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Mon, 21 Dec 2020 22:14:24 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10374: Limit iteration at BufferedTupleStream::DebugString
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16884 ) Change subject: IMPALA-10374: Limit iteration at BufferedTupleStream::DebugString .. Patch Set 2: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/7891/ : 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/16884 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6626c8d54f35f303c01f85be1dd9aa54c8ad9a2d Gerrit-Change-Number: 16884 Gerrit-PatchSet: 2 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Mon, 21 Dec 2020 22:12:19 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10325: Parquet scan should use min/max statistics to skip pages based on equi-join predicate
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16720 ) Change subject: IMPALA-10325: Parquet scan should use min/max statistics to skip pages based on equi-join predicate .. Patch Set 38: (2 comments) http://gerrit.cloudera.org:8080/#/c/16720/38/tests/run-tests.py File tests/run-tests.py: http://gerrit.cloudera.org:8080/#/c/16720/38/tests/run-tests.py@216 PS38, Line 216: # flake8: E265 block comment should start with '# ' http://gerrit.cloudera.org:8080/#/c/16720/38/tests/run-tests.py@219 PS38, Line 219: % flake8: E131 continuation line unaligned for hanging indent -- To view, visit http://gerrit.cloudera.org:8080/16720 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I379405ee75b14929df7d6b5d20dabc6f51375691 Gerrit-Change-Number: 16720 Gerrit-PatchSet: 38 Gerrit-Owner: Qifan Chen Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Mon, 21 Dec 2020 22:09:42 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10325: Parquet scan should use min/max statistics to skip pages based on equi-join predicate
Qifan Chen has uploaded a new patch set (#38). ( http://gerrit.cloudera.org:8080/16720 ) Change subject: IMPALA-10325: Parquet scan should use min/max statistics to skip pages based on equi-join predicate .. IMPALA-10325: Parquet scan should use min/max statistics to skip pages based on equi-join predicate This patch adds a new class of predicates called overlap predicates to aid in the determination of whether a Parquet row group or a page overlap with a range computed from an equi hash join. If not, then the entire row group or page are skipped. An overlap predicate exists as a min/max filter. For the following query, the min and max in such a min/max filter are computed with the values from the join column from table 'b' and become fully available when the entire hash table is built. To evaluate the overlap predicate, these two values are compared against the min/max of each row group or page at the scan node for 'a'. select straight_join count(*) from lineitem_sorted_l_shipdate a join [SHUFFLE] lineitem_sorted_l_shipdate b where a.l_shipdate = b.l_receiptdate and b.l_commitdate = "1992-01-31"; An overlap predicate associated with the column type J (in hash table) and scan column type S will be formed when one of the following is true: Both J and S are booleans Both J and S are integers (tinyint, smallint, int, or bigint) Both J and S are approximate numeric (float or double) Both J and S are decimals with the same precision and scale Both J and S are strings (STRING, CHAR or VARCHAR) Both J and S are date Both J and S are timestamp Like any existing min/max filters, MAX_NUM_RUNTIME_FILTERS query option does not apply to min/max filters created for overlap predicates. The overlap predicates will always be evaluated, after the min/max conjuncts (if any). Two new run-time profile counters are added to report the number of row groups or pages filtered out via the overlap predicates respectively: 1. NumRuntimeFilteredRowGroups 2. NumRuntimeFilteredPages Testing: 1. Unit tested on various column types with TPCH and TPCDS tables. Benefits were significant when the join column on the outer table is sorted, or when the min/max boundary values of the pages or row groups are monotonic; 2. Added new tests in min_max_filters.test to demonstrate the number of filtered out pages and row groups with the two new profile counters; 2. Added new tests in runtime-filter-propagation.test to demonstrate that the overlap predicates work with different column types; 4. Added data type specific overlap method tests in min-max-filter-test.cc; 5. Core testing. TBD in this patch: 1. Performance measurement. To do in follow-up JIRAs: 1. Apply the overlap predicate on partition columns; 2. IR code-gen for various MinMaxFilter::EvalOverlap methods. Change-Id: I379405ee75b14929df7d6b5d20dabc6f51375691 --- M be/src/exec/exec-node.h M be/src/exec/hdfs-scan-node-base.cc M be/src/exec/hdfs-scan-node-base.h M be/src/exec/parquet/hdfs-parquet-scanner.cc M be/src/exec/parquet/hdfs-parquet-scanner.h M be/src/exec/parquet/parquet-column-stats.cc M be/src/exec/parquet/parquet-column-stats.h M be/src/exec/partitioned-hash-join-builder.cc M be/src/exec/scan-node.cc M be/src/runtime/coordinator.cc M be/src/runtime/date-value.cc M be/src/runtime/date-value.h M be/src/runtime/runtime-filter-ir.cc M be/src/runtime/timestamp-value.cc M be/src/runtime/timestamp-value.h M be/src/service/query-options.cc M be/src/service/query-options.h M be/src/util/min-max-filter-test.cc M be/src/util/min-max-filter.cc M be/src/util/min-max-filter.h M common/thrift/ImpalaInternalService.thrift M common/thrift/ImpalaService.thrift M common/thrift/PlanNodes.thrift M fe/src/main/java/org/apache/impala/analysis/TupleDescriptor.java M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java M fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java M fe/src/test/java/org/apache/impala/planner/PlannerTest.java A testdata/workloads/functional-planner/queries/PlannerTest/disable-runtime-overlap-filter.test M testdata/workloads/functional-planner/queries/PlannerTest/runtime-filter-propagation.test M testdata/workloads/functional-query/queries/QueryTest/min_max_filters.test M tests/run-tests.py 31 files changed, 1,828 insertions(+), 239 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/16720/38 -- To view, visit http://gerrit.cloudera.org:8080/16720 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I379405ee75b14929df7d6b5d20dabc6f51375691 Gerrit-Change-Number: 16720 Gerrit-PatchSet: 38 Gerrit-Owner: Qifan Chen Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan
[Impala-ASF-CR] IMPALA-10374: Limit iteration at BufferedTupleStream::DebugString
Hello Quanlong Huang, Csaba Ringhofer, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16884 to look at the new patch set (#2). Change subject: IMPALA-10374: Limit iteration at BufferedTupleStream::DebugString .. IMPALA-10374: Limit iteration at BufferedTupleStream::DebugString BufferedTupleStream::DebugString() iterate std::list that can potentially grow very large. As consequent, the returned string can grow large as well and cause a problem as previously happen in IMPALA-9851. With this patch, BufferedTupleStream::DebugString() only include maximum of 100 first pages of page list. Testing: - Add new be test SimpleTupleStreamTest.ShortDebugString in buffered-tuple-stream-test.cc - Pass core tests Change-Id: I6626c8d54f35f303c01f85be1dd9aa54c8ad9a2d --- M be/src/runtime/buffered-tuple-stream-test.cc M be/src/runtime/buffered-tuple-stream.cc M be/src/runtime/buffered-tuple-stream.h 3 files changed, 73 insertions(+), 4 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/84/16884/2 -- To view, visit http://gerrit.cloudera.org:8080/16884 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I6626c8d54f35f303c01f85be1dd9aa54c8ad9a2d Gerrit-Change-Number: 16884 Gerrit-PatchSet: 2 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang Gerrit-Reviewer: Riza Suminto
[Impala-ASF-CR] IMPALA-9966: Add missing breaks in SetQueryOption
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16892 ) Change subject: IMPALA-9966: Add missing breaks in SetQueryOption .. Patch Set 1: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/7890/ : 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/16892 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I696204c7036c0059fe4eed2d23c8e947b6c53d8d Gerrit-Change-Number: 16892 Gerrit-PatchSet: 1 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Mon, 21 Dec 2020 20:57:14 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9793: Impala quickstart cluster with docker-compose
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15966 ) Change subject: IMPALA-9793: Impala quickstart cluster with docker-compose .. Patch Set 9: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/7889/ : 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/15966 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifc0b862af40a368381ada7ec2a355fe4b0aa778c Gerrit-Change-Number: 15966 Gerrit-PatchSet: 9 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Quanlong Huang Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 21 Dec 2020 20:46:57 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9975 (part 1): Various refactors for admission control daemon
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/16890 ) Change subject: IMPALA-9975 (part 1): Various refactors for admission control daemon .. Patch Set 1: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/16890 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2e097e20458354f78bfc3477cac6fb3a2835f094 Gerrit-Change-Number: 16890 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Wenzhe Zhou Gerrit-Comment-Date: Mon, 21 Dec 2020 20:37:40 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9966: Add missing breaks in SetQueryOption
Tim Armstrong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/16892 Change subject: IMPALA-9966: Add missing breaks in SetQueryOption .. IMPALA-9966: Add missing breaks in SetQueryOption Change-Id: I696204c7036c0059fe4eed2d23c8e947b6c53d8d --- M be/src/service/query-options.cc 1 file changed, 7 insertions(+), 5 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/92/16892/1 -- To view, visit http://gerrit.cloudera.org:8080/16892 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I696204c7036c0059fe4eed2d23c8e947b6c53d8d Gerrit-Change-Number: 16892 Gerrit-PatchSet: 1 Gerrit-Owner: Tim Armstrong
[Impala-ASF-CR] IMPALA-9793: Impala quickstart cluster with docker-compose
Hello Quanlong Huang, Grant Henke, Joe McDonnell, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/15966 to look at the new patch set (#9). Change subject: IMPALA-9793: Impala quickstart cluster with docker-compose .. IMPALA-9793: Impala quickstart cluster with docker-compose What works: * A single node cluster can be started up with docker-compose * HMS data is stored in Derby database in a docker volume * Filesystem data is stored in a shared docker volume, using the localfs support in the Hadoop client. * A Kudu cluster with a single master can be optionally added on to the Impala cluster. * TPC-DS data can be loaded automatically by a data loading container. We need to set up a docker network called quickstart-network, purely because docker-compose insists on generating network names with underscores, which are part of the FQDN and end up causing problems with Java's URL parsing, which rejects these technically invalid domain names. How to run: Instructions for running the quickstart cluster are in docker/quickstart.yml. How to build containers: ./buildall.sh -release -noclean -notests -ninja ninja quickstart_hms_image quickstart_client_image docker_images Misc other stuff: * Added more metadata to all images. TODO: * Test and instructions to run against Kudu quickstart * Upload latest version of containers before merging. Change-Id: Ifc0b862af40a368381ada7ec2a355fe4b0aa778c --- M docker/CMakeLists.txt A docker/docker-build.sh M docker/impala_base/Dockerfile A docker/quickstart-kudu-minimal.yml A docker/quickstart-load-data.yml A docker/quickstart.yml A docker/quickstart_client/Dockerfile A docker/quickstart_client/data-load-entrypoint.sh A docker/quickstart_client/load_tpcds_kudu.sql A docker/quickstart_client/load_tpcds_parquet.sql A docker/quickstart_conf/hive-site.xml A docker/quickstart_hms/Dockerfile A docker/quickstart_hms/hms-entrypoint.sh 13 files changed, 2,913 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/66/15966/9 -- To view, visit http://gerrit.cloudera.org:8080/15966 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ifc0b862af40a368381ada7ec2a355fe4b0aa778c Gerrit-Change-Number: 15966 Gerrit-PatchSet: 9 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Quanlong Huang Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-9975 (part 2): Introduce new admission control daemon
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16891 ) Change subject: IMPALA-9975 (part 2): Introduce new admission control daemon .. Patch Set 1: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/7888/ : 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/16891 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id677814b31e9193035e8cf0d08aba0ce388a0ad9 Gerrit-Change-Number: 16891 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 21 Dec 2020 19:52:59 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9975 (part 1): Various refactors for admission control daemon
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16890 ) Change subject: IMPALA-9975 (part 1): Various refactors for admission control daemon .. Patch Set 1: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/7887/ : 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/16890 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2e097e20458354f78bfc3477cac6fb3a2835f094 Gerrit-Change-Number: 16890 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 21 Dec 2020 19:52:50 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9975 (part 2): Introduce new admission control daemon
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16891 ) Change subject: IMPALA-9975 (part 2): Introduce new admission control daemon .. Patch Set 1: (6 comments) http://gerrit.cloudera.org:8080/#/c/16891/1/bin/start-impala-cluster.py File bin/start-impala-cluster.py: http://gerrit.cloudera.org:8080/#/c/16891/1/bin/start-impala-cluster.py@74 PS1, Line 74: t flake8: E501 line too long (98 > 90 characters) http://gerrit.cloudera.org:8080/#/c/16891/1/bin/start-impala-cluster.py@293 PS1, Line 293: def build_admissiond_arg_list(): flake8: E302 expected 2 blank lines, found 1 http://gerrit.cloudera.org:8080/#/c/16891/1/tests/common/impala_cluster.py File tests/common/impala_cluster.py: http://gerrit.cloudera.org:8080/#/c/16891/1/tests/common/impala_cluster.py@106 PS1, Line 106: s flake8: E501 line too long (102 > 90 characters) http://gerrit.cloudera.org:8080/#/c/16891/1/tests/common/impala_cluster.py@236 PS1, Line 236: i flake8: E501 line too long (101 > 90 characters) http://gerrit.cloudera.org:8080/#/c/16891/1/tests/common/impala_cluster.py@566 PS1, Line 566: class AdmissiondProcess(BaseImpalaProcess): flake8: E302 expected 2 blank lines, found 1 http://gerrit.cloudera.org:8080/#/c/16891/1/tests/common/impala_cluster.py@571 PS1, Line 571: # flake8: E265 block comment should start with '# ' -- To view, visit http://gerrit.cloudera.org:8080/16891 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id677814b31e9193035e8cf0d08aba0ce388a0ad9 Gerrit-Change-Number: 16891 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 21 Dec 2020 19:32:21 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9975 (part 1): Various refactors for admission control daemon
Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/16890 ) Change subject: IMPALA-9975 (part 1): Various refactors for admission control daemon .. Patch Set 1: It may be helpful to look at this patch first: https://gerrit.cloudera.org/#/c/16891/ to understand the context here -- To view, visit http://gerrit.cloudera.org:8080/16890 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2e097e20458354f78bfc3477cac6fb3a2835f094 Gerrit-Change-Number: 16890 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 21 Dec 2020 19:31:56 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9975 (part 2): Introduce new admission control daemon
Thomas Tauber-Marshall has uploaded this change for review. ( http://gerrit.cloudera.org:8080/16891 Change subject: IMPALA-9975 (part 2): Introduce new admission control daemon .. IMPALA-9975 (part 2): Introduce new admission control daemon A recent patch (IMPALA-9930) introduces a new admission control rpc service, which can be configured to perform admission control for coordinators. In that patch, the admission service runs in an impalad. This patch separates the service out to run in a new daemon, called the admissiond. Some notable changes: - Adds a new class, AdmissiondEnv, which performs the same function for the admissiond as ExecEnv does for impalads. - The '/admission' http endpoint is exposed on the admissiond's webui if the admission control service is in use, otherwise it is exposed on coordinator impalad's webuis. - Executors now never create an AdmissionController, and coordinators do not create one if configured to use an admissiond. This requires removing admission related info on the '/backends' endpoint and completely removing the '/admission' endpoint for these daemons. - start-impala-cluster.py takes a new flag --enable_admission_service which configures the minicluster to have an admissiond with all coordinators using it for admisison control. TODO: integrate this with the Docker infrastructure Testing: - Existing tests for the admission control serivce are modified to run with an admissiond. Change-Id: Id677814b31e9193035e8cf0d08aba0ce388a0ad9 --- M CMakeLists.txt M be/src/runtime/exec-env.cc M be/src/runtime/exec-env.h M be/src/scheduling/CMakeLists.txt M be/src/scheduling/admission-control-client.cc M be/src/scheduling/admission-control-service.cc M be/src/scheduling/admission-control-service.h A be/src/scheduling/admissiond-env.cc A be/src/scheduling/admissiond-env.h A be/src/scheduling/admissiond-main.cc M be/src/service/CMakeLists.txt M be/src/service/daemon-main.cc M be/src/service/impala-http-handler.cc M be/src/service/impala-http-handler.h M be/src/service/impala-server.cc M be/src/util/mem-info.cc M be/src/util/mem-info.h M bin/start-impala-cluster.py M common/thrift/metrics.json M tests/common/custom_cluster_test_suite.py M tests/common/impala_cluster.py M tests/common/impala_service.py M tests/custom_cluster/test_admission_controller.py M www/backends.tmpl 24 files changed, 574 insertions(+), 185 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/91/16891/1 -- To view, visit http://gerrit.cloudera.org:8080/16891 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Id677814b31e9193035e8cf0d08aba0ce388a0ad9 Gerrit-Change-Number: 16891 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Tauber-Marshall
[Impala-ASF-CR] IMPALA-9975 (part 1): Various refactors for admission control daemon
Thomas Tauber-Marshall has uploaded this change for review. ( http://gerrit.cloudera.org:8080/16890 Change subject: IMPALA-9975 (part 1): Various refactors for admission control daemon .. IMPALA-9975 (part 1): Various refactors for admission control daemon This patch contains a variety of small refactors needed to enable the new admission control daemon, to separate them out from the main patch for ease of reviewing. The following changes are made: - A new class is introduced, DaemonEnv, which contains singleton objects common to all Impala daemons, currently just a MetricGroup and Webservers. The purpose is to reduce code duplication when the new admissiond daemon is added. This is analogous to how ExecEnv is used for impalad-specific singletons already. This patch modifies the catalogd and statestored to use DaemonEnv. impalads could also use DaemonEnv, but its tricky due to dependencies in the order of creation and initialization for objects such as the ReservationTracker and BufferPool relative to the MetricGroup and Webserver, so this is left for followup work. - Direct use of ExecEnv in ImpalaServicePool ahd AdmissionController is removed, as the admissiond will also need to use these classes and it will not have an ExecEnv. Testing: - Passed a run of existing core tests. Change-Id: I2e097e20458354f78bfc3477cac6fb3a2835f094 --- M be/src/catalog/catalogd-main.cc M be/src/common/CMakeLists.txt A be/src/common/daemon-env.cc A be/src/common/daemon-env.h M be/src/rpc/impala-service-pool.cc M be/src/rpc/impala-service-pool.h M be/src/rpc/rpc-mgr-kerberized-test.cc M be/src/rpc/rpc-mgr-test.cc M be/src/rpc/rpc-mgr-test.h M be/src/rpc/rpc-mgr.cc M be/src/rpc/rpc-mgr.h M be/src/runtime/data-stream-test.cc M be/src/runtime/exec-env.cc M be/src/scheduling/admission-control-service.cc M be/src/scheduling/admission-controller-test.cc M be/src/scheduling/admission-controller.cc M be/src/scheduling/admission-controller.h M be/src/service/control-service.cc M be/src/service/data-stream-service.cc M be/src/statestore/statestored-main.cc 20 files changed, 215 insertions(+), 112 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/90/16890/1 -- To view, visit http://gerrit.cloudera.org:8080/16890 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I2e097e20458354f78bfc3477cac6fb3a2835f094 Gerrit-Change-Number: 16890 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Tauber-Marshall
[Impala-ASF-CR] IMPALA-10336: Coordinator return incorrect error to client
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16849 ) Change subject: IMPALA-10336: Coordinator return incorrect error to client .. Patch Set 4: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/6797/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/16849 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I75f252e43006c6ff6980800e3254672de396b318 Gerrit-Change-Number: 16849 Gerrit-PatchSet: 4 Gerrit-Owner: Wenzhe Zhou Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Wenzhe Zhou Gerrit-Comment-Date: Mon, 21 Dec 2020 19:25:24 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10336: Coordinator return incorrect error to client
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16849 ) Change subject: IMPALA-10336: Coordinator return incorrect error to client .. Patch Set 4: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/16849 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I75f252e43006c6ff6980800e3254672de396b318 Gerrit-Change-Number: 16849 Gerrit-PatchSet: 4 Gerrit-Owner: Wenzhe Zhou Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Wenzhe Zhou Gerrit-Comment-Date: Mon, 21 Dec 2020 19:25:23 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10336: Coordinator return incorrect error to client
Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/16849 ) Change subject: IMPALA-10336: Coordinator return incorrect error to client .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/16849 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I75f252e43006c6ff6980800e3254672de396b318 Gerrit-Change-Number: 16849 Gerrit-PatchSet: 3 Gerrit-Owner: Wenzhe Zhou Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Wenzhe Zhou Gerrit-Comment-Date: Mon, 21 Dec 2020 19:25:09 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9865: part 2/2: add verbosity to profile tool
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/16881 ) Change subject: IMPALA-9865: part 2/2: add verbosity to profile tool .. Patch Set 6: (5 comments) http://gerrit.cloudera.org:8080/#/c/16881/6/be/src/util/impala-profile-tool.cc File be/src/util/impala-profile-tool.cc: http://gerrit.cloudera.org:8080/#/c/16881/6/be/src/util/impala-profile-tool.cc@88 PS6, Line 88: RuntimeProfileBase::Verbosity configured_verbosity; Is this meant to be initialized as RuntimeProfileBase::Verbosity::LEGACY ? http://gerrit.cloudera.org:8080/#/c/16881/6/be/src/util/impala-profile-tool.cc@136 PS6, Line 136: verbosity = profile_version < 2 ? RuntimeProfileBase::Verbosity::LEGACY : In case of v1 profile input and --profile_verbosity=default, this line does not seem to override it back to Verbosity::LEGACY. Does this implies that both v1 and v2 profile input can be parsed into any verbosity level? http://gerrit.cloudera.org:8080/#/c/16881/6/bin/rat_exclude_files.txt File bin/rat_exclude_files.txt: http://gerrit.cloudera.org:8080/#/c/16881/6/bin/rat_exclude_files.txt@157 PS6, Line 157: testdata/impala-profiles/impala_profile_log_tpcds_compute_stats.expected Should we add `.txt` suffix for expected output in text format? http://gerrit.cloudera.org:8080/#/c/16881/6/tests/observability/test_profile_tool.py File tests/observability/test_profile_tool.py: http://gerrit.cloudera.org:8080/#/c/16881/6/tests/observability/test_profile_tool.py@36 PS6, Line 36: self._compare_profile_tool_output([], Since default verbosity might change in the future, can we add a test using argument `--profile_verbosity=legacy` ? Should we also test between v1 vs v2 profile input when --profile_verbosity is not specified? http://gerrit.cloudera.org:8080/#/c/16881/6/tests/observability/test_profile_tool.py@38 PS6, Line 38: get_profile_path('impala_profile_log_tpcds_compute_stats.expected')) Rename expected file name to "impala_profile_log_tpcds_compute_stats_legacy.expected" for clarity? -- To view, visit http://gerrit.cloudera.org:8080/16881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I82618a813e29af7996dfaed78873b2a73bc0231d Gerrit-Change-Number: 16881 Gerrit-PatchSet: 6 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 21 Dec 2020 19:18:57 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6671: Skip locked tables from topic updates
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16549 ) Change subject: IMPALA-6671: Skip locked tables from topic updates .. Patch Set 17: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/6796/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/16549 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic657b96edbcdc94c6b906e7ca59291f4e4715655 Gerrit-Change-Number: 16549 Gerrit-PatchSet: 17 Gerrit-Owner: Vihang Karajgaonkar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang Gerrit-Reviewer: Shant Hovsepian Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Mon, 21 Dec 2020 18:44:28 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6671: Skip locked tables from topic updates
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16549 ) Change subject: IMPALA-6671: Skip locked tables from topic updates .. Patch Set 17: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/16549 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic657b96edbcdc94c6b906e7ca59291f4e4715655 Gerrit-Change-Number: 16549 Gerrit-PatchSet: 17 Gerrit-Owner: Vihang Karajgaonkar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang Gerrit-Reviewer: Shant Hovsepian Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Mon, 21 Dec 2020 18:44:28 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6671: Skip locked tables from topic updates
Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16549 ) Change subject: IMPALA-6671: Skip locked tables from topic updates .. Patch Set 16: Code-Review+2 There were minor updates since the last update. Carrying forward the votes from Tim and Quanlong. -- To view, visit http://gerrit.cloudera.org:8080/16549 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic657b96edbcdc94c6b906e7ca59291f4e4715655 Gerrit-Change-Number: 16549 Gerrit-PatchSet: 16 Gerrit-Owner: Vihang Karajgaonkar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang Gerrit-Reviewer: Shant Hovsepian Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Mon, 21 Dec 2020 18:36:52 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6671: Skip locked tables from topic updates
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16549 ) Change subject: IMPALA-6671: Skip locked tables from topic updates .. Patch Set 16: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/7886/ : 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/16549 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic657b96edbcdc94c6b906e7ca59291f4e4715655 Gerrit-Change-Number: 16549 Gerrit-PatchSet: 16 Gerrit-Owner: Vihang Karajgaonkar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang Gerrit-Reviewer: Shant Hovsepian Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Mon, 21 Dec 2020 18:27:33 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6671: Skip locked tables from topic updates
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16549 ) Change subject: IMPALA-6671: Skip locked tables from topic updates .. Patch Set 16: (3 comments) http://gerrit.cloudera.org:8080/#/c/16549/16/tests/custom_cluster/test_topic_update_frequency.py File tests/custom_cluster/test_topic_update_frequency.py: http://gerrit.cloudera.org:8080/#/c/16549/16/tests/custom_cluster/test_topic_update_frequency.py@54 PS16, Line 54: l flake8: E501 line too long (146 > 90 characters) http://gerrit.cloudera.org:8080/#/c/16549/16/tests/custom_cluster/test_topic_update_frequency.py@120 PS16, Line 120: f flake8: E126 continuation line over-indented for hanging indent http://gerrit.cloudera.org:8080/#/c/16549/16/tests/custom_cluster/test_topic_update_frequency.py@152 PS16, Line 152: l flake8: E501 line too long (146 > 90 characters) -- To view, visit http://gerrit.cloudera.org:8080/16549 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic657b96edbcdc94c6b906e7ca59291f4e4715655 Gerrit-Change-Number: 16549 Gerrit-PatchSet: 16 Gerrit-Owner: Vihang Karajgaonkar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang Gerrit-Reviewer: Shant Hovsepian Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Mon, 21 Dec 2020 18:06:42 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6671: Skip locked tables from topic updates
Vihang Karajgaonkar has uploaded a new patch set (#16). ( http://gerrit.cloudera.org:8080/16549 ) Change subject: IMPALA-6671: Skip locked tables from topic updates .. IMPALA-6671: Skip locked tables from topic updates This change adds a mechanism for topic-update thread to skip a table which is locked for more than a configurable interval from the topic updates. This is especially useful in scenarios where long running operations on a locked table (refresh, recover partitions, compute stats) block the topic update thread. This causes unrelated queries which are waiting on metadata via topic updates (catalog-v1 mode) to unnecessarily block. The ideal solution of this problem would be to make HdfsTable immutable so that there is no need for table lock. But that is large change and not easily portable to older releases of Impala. It would be taken up as a separate patch. This change introduces 2 new configurations for catalogd: 1. topic_update_tbl_max_wait_time_ms: This defines the maximum time in msecs the topic update thread waits on a locked table before skipping the table from that iteration of topic updates. The default value is 500. If this configuration is set to 0 the lock with timeout for topic update thread is disabled. 2. catalog_max_lock_skipped_topic_updates: This defines the maximum number of distinct lock operations which are skipped by topic update thread due to lock contention. Once this limit is reached, topic update thread will block until it acquires the table lock and adds it to the updates. Testing: 1. Added a test case which introduces a simulated delay in a few potentially long running statements. This causes the table to be locked for a long time. The topic update thread skips that table from updates and unrelated queries are unblocked since they receive the required metadata from updates. 2. Added a test where multiple threads run blocking statements in a loop to stress the table lock. It makes sure that topic update thread is not starved and eventually blocks on table lock by hitting the limit defined by catalog_max_lock_skipped_topic_updates. 3. Ran exhaustive tests with default configurations. Change-Id: Ic657b96edbcdc94c6b906e7ca59291f4e4715655 --- M be/src/catalog/catalog-server.cc M be/src/util/backend-gflag-util.cc M common/thrift/BackendGflags.thrift M fe/src/main/java/org/apache/impala/analysis/CopyTestCaseStmt.java M fe/src/main/java/org/apache/impala/catalog/Catalog.java M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java M fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java M fe/src/main/java/org/apache/impala/catalog/Table.java M fe/src/main/java/org/apache/impala/catalog/TopicUpdateLog.java M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java M fe/src/test/java/org/apache/impala/catalog/CatalogObjectToFromThriftTest.java A tests/custom_cluster/test_topic_update_frequency.py 13 files changed, 698 insertions(+), 149 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/49/16549/16 -- To view, visit http://gerrit.cloudera.org:8080/16549 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ic657b96edbcdc94c6b906e7ca59291f4e4715655 Gerrit-Change-Number: 16549 Gerrit-PatchSet: 16 Gerrit-Owner: Vihang Karajgaonkar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang Gerrit-Reviewer: Shant Hovsepian Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vihang Karajgaonkar
[Impala-ASF-CR] IMPALA-6671: Skip locked tables from topic updates
Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16549 ) Change subject: IMPALA-6671: Skip locked tables from topic updates .. Patch Set 15: (7 comments) http://gerrit.cloudera.org:8080/#/c/16549/15/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java: http://gerrit.cloudera.org:8080/#/c/16549/15/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1311 PS15, Line 1311: to > nit: to get Done http://gerrit.cloudera.org:8080/#/c/16549/15/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1325 PS15, Line 1325: LOG.warn("Topic update thread blocking until lock is acquired for table {}", > This is too verbose when topicUpdateTblLockMaxWaitTimeMs_ is 0. Almost all makes sense. Done. http://gerrit.cloudera.org:8080/#/c/16549/15/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2235 PS15, Line 2235: the > nit: remove "the" Done http://gerrit.cloudera.org:8080/#/c/16549/15/tests/custom_cluster/test_topic_update_frequency.py File tests/custom_cluster/test_topic_update_frequency.py: http://gerrit.cloudera.org:8080/#/c/16549/15/tests/custom_cluster/test_topic_update_frequency.py@20 PS15, Line 20: @SkipIfS3.variable_listing_times > flake8: E302 expected 2 blank lines, found 1 Done http://gerrit.cloudera.org:8080/#/c/16549/15/tests/custom_cluster/test_topic_update_frequency.py@66 PS15, Line 66: is > nit: without? Thanks for pointing this out. http://gerrit.cloudera.org:8080/#/c/16549/15/tests/custom_cluster/test_topic_update_frequency.py@70 PS15, Line 70: is > nit: with? Done http://gerrit.cloudera.org:8080/#/c/16549/15/tests/custom_cluster/test_topic_update_frequency.py@106 PS15, Line 106: # there is no other good way other than to wait for some time to make sure that : # the slow query has been submitted and being compiled before we start the : # non_blocking queries : time.sleep(1) > Just for comments, we can use "wait_for_state" to wait for the CREATE state Actually, I had used this before in my earlier attempt but I found that to be flaky. My understanding is the CREATE state will be set when admission controller accepts the query? There is still a time delay between when the query is in create state and the query takes the table lock in catalogd. -- To view, visit http://gerrit.cloudera.org:8080/16549 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic657b96edbcdc94c6b906e7ca59291f4e4715655 Gerrit-Change-Number: 16549 Gerrit-PatchSet: 15 Gerrit-Owner: Vihang Karajgaonkar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang Gerrit-Reviewer: Shant Hovsepian Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Mon, 21 Dec 2020 17:50:11 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9865: part 2/2: add verbosity to profile tool
Tim Armstrong has removed a vote on this change. Change subject: IMPALA-9865: part 2/2: add verbosity to profile tool .. Removed Verified-1 by Impala Public Jenkins -- To view, visit http://gerrit.cloudera.org:8080/16881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: I82618a813e29af7996dfaed78873b2a73bc0231d Gerrit-Change-Number: 16881 Gerrit-PatchSet: 6 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-9865: part 2/2: add verbosity to profile tool
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16881 ) Change subject: IMPALA-9865: part 2/2: add verbosity to profile tool .. Patch Set 6: The -1 is a minor clang-tidy issue. -- To view, visit http://gerrit.cloudera.org:8080/16881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I82618a813e29af7996dfaed78873b2a73bc0231d Gerrit-Change-Number: 16881 Gerrit-PatchSet: 6 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 21 Dec 2020 17:30:40 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10317: Add query option that limits huge joins at runtime
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16706 ) Change subject: IMPALA-10317: Add query option that limits huge joins at runtime .. Patch Set 6: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/7885/ : 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/16706 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idbca7e053b61b4e31b066edcfb3b0398fa859d02 Gerrit-Change-Number: 16706 Gerrit-PatchSet: 6 Gerrit-Owner: Fucun Chu Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Fucun Chu Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Mon, 21 Dec 2020 15:06:06 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10317: Add query option that limits huge joins at runtime
Fucun Chu has posted comments on this change. ( http://gerrit.cloudera.org:8080/16706 ) Change subject: IMPALA-10317: Add query option that limits huge joins at runtime .. Patch Set 6: (3 comments) http://gerrit.cloudera.org:8080/#/c/16706/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16706/5//COMMIT_MSG@9 PS5, Line 9: a join no > You may want to mention why it is useful even though it may be somewhat obv Done http://gerrit.cloudera.org:8080/#/c/16706/3/be/src/runtime/coordinator.h File be/src/runtime/coordinator.h: http://gerrit.cloudera.org:8080/#/c/16706/3/be/src/runtime/coordinator.h@230 PS3, Line 230: /// Total num rows produced by each join node. The key is join node id. > Good ! Done http://gerrit.cloudera.org:8080/#/c/16706/5/common/protobuf/control_service.proto File common/protobuf/control_service.proto: http://gerrit.cloudera.org:8080/#/c/16706/5/common/protobuf/control_service.proto@261 PS5, Line 261: map per_join_rows_produced = 16; > Mark this as 'optional' field ? Maps cannot be repeated, optional, or required. from: https://developers.google.com/protocol-buffers/docs/proto#maps -- To view, visit http://gerrit.cloudera.org:8080/16706 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idbca7e053b61b4e31b066edcfb3b0398fa859d02 Gerrit-Change-Number: 16706 Gerrit-PatchSet: 6 Gerrit-Owner: Fucun Chu Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Fucun Chu Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Mon, 21 Dec 2020 14:44:58 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10317: Add query option that limits huge joins at runtime
Fucun Chu has uploaded a new patch set (#6). ( http://gerrit.cloudera.org:8080/16706 ) Change subject: IMPALA-10317: Add query option that limits huge joins at runtime .. IMPALA-10317: Add query option that limits huge joins at runtime This patch adds support for limiting the rows produced by a join node such that runaway join queries can be prevented. The limit is specified by a query option. Queries exceeding that limit get terminated. The checking runs periodically, so the actual rows produced may go somewhat over the limit. JOIN_ROWS_PRODUCED_LIMIT is exposed as an advanced query option. Rows produced Query profile is updated to include query wide and per backend metrics for RowsReturned. Example from " set JOIN_ROWS_PRODUCED_LIMIT = 1000; select count(*) from tpch_parquet.lineitem l1 cross join (select * from tpch_parquet.lineitem l2 limit 5) l3;": NESTED_LOOP_JOIN_NODE (id=2): - InactiveTotalTime: 107.534ms - PeakMemoryUsage: 16.00 KB (16384) - ProbeRows: 1.02K (1024) - ProbeTime: 0.000ns - RowsReturned: 10.00M (10002025) - RowsReturnedRate: 749.58 K/sec - TotalTime: 13s337ms Testing: Added tests for JOIN_ROWS_PRODUCED_LIMIT Change-Id: Idbca7e053b61b4e31b066edcfb3b0398fa859d02 --- M be/src/runtime/coordinator-backend-state.cc M be/src/runtime/coordinator.h M be/src/runtime/fragment-instance-state.cc M be/src/runtime/fragment-instance-state.h M be/src/runtime/query-state.cc M be/src/service/impala-server.cc M be/src/service/query-options-test.cc M be/src/service/query-options.cc M be/src/service/query-options.h M common/protobuf/control_service.proto M common/thrift/ImpalaInternalService.thrift M common/thrift/ImpalaService.thrift M common/thrift/generate_error_codes.py M testdata/workloads/functional-query/queries/QueryTest/query-resource-limits.test 14 files changed, 167 insertions(+), 5 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/06/16706/6 -- To view, visit http://gerrit.cloudera.org:8080/16706 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Idbca7e053b61b4e31b066edcfb3b0398fa859d02 Gerrit-Change-Number: 16706 Gerrit-PatchSet: 6 Gerrit-Owner: Fucun Chu Gerrit-Reviewer: Aman Sinha Gerrit-Reviewer: Fucun Chu Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-10211 (Part 1): Add support for role-related statements
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16837 ) Change subject: IMPALA-10211 (Part 1): Add support for role-related statements .. Patch Set 13: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/16837 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic2b204e62a1d8ae1932d955b4efc28be22202860 Gerrit-Change-Number: 16837 Gerrit-PatchSet: 13 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang Gerrit-Comment-Date: Mon, 21 Dec 2020 14:29:51 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10211 (Part 1): Add support for role-related statements
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/16837 ) Change subject: IMPALA-10211 (Part 1): Add support for role-related statements .. IMPALA-10211 (Part 1): Add support for role-related statements This patch adds the support for the following role-related statements. 1. CREATE ROLE . 2. DROP ROLE . 3. GRANT ROLE TO GROUP . 4. REVOKE ROLE FROM GROUP . 5. GRANT ON TO ROLE . 6. REVOKE ON FROM ROLE . 7. SHOW GRANT ROLE ON . 8. SHOW ROLES. 9. SHOW CURRENT ROLES. 10. SHOW ROLE GRANT GROUP . To support the first 4 statements, we implemented the methods of createRole()/dropRole(), and grantRoleToGroup()/revokeRoleFromGroup() with their respective API calls provided by Ranger. To support the 5th and 6th statements, we modified createGrantRevokeRequest() so that the cases in which the grantee or revokee is a role could be processed. We slightly extended getPrivileges() so as to include the case when the principal is a role for the 7th statement. For the last 3 statements, to make Impala's behavior consistent with that when Sentry was the authorization provider, we based our implementation on SentryImpaladAuthorizationManager#getRoles() at https://gerrit.cloudera.org/c/15833/8/fe/src/main/java/org/apache/impala/authorization/sentry/SentryImpaladAuthorizationManager.java, which was removed in IMPALA-9708 when we dropped the support for Sentry. To test the implemented functionalities, we based our test cases on those at https://gerrit.cloudera.org/c/15833/8/testdata/workloads/functional-query/queries/QueryTest/grant_revoke.test. We note that before our tests could be automatically run in a Kerberized environment (IMPALA-9360), in order to run the statements of CREATE/DROP ROLE , GRANT/REVOKE ROLE TO/FROM GROUP , and SHOW ROLES, we revised security-applicationContext.xml, one of the files needed when the Ranger server is started, so that the corresponding API calls could be performed in a non-Kerberized environment. During the process of adding test cases to grant_revoke.test, we found the following differences in Impala's behavior between the case when Ranger is the authorization provider and that when Sentry is the authorization provider. Specifically, we have the following two major differences. 1. Before dropping a role in Ranger, we have to remove all the privileges granted to the role in advance, which is not the case when Sentry is the authorization provider. 2. The resource has to be specified for the statement of SHOW GRANT ROLE ON , which is different when Sentry is the authorization provider. This could be partly due to the fact that there is no API provided by Ranger that allows Impala to directly retrieve the list of all privileges granted to a specified role. Due to the differences in Impala's behavior described above, we had to revise the test cases in grant_revoke.test accordingly. On the other hand, to include as many test cases that were in the original grant_revoke.test as possible, we had to explicitly add the test section of 'USER' to specify the connecting user to Impala for some queries that require the connecting user to be a Ranger administrator, e.g., CREATE/DROP ROLE and GRANT/REVOKE TO/FROM GROUP . The user has to be 'admin' in the current grant_revoke.test, whereas it could be the default user 'getuser()' in the original grant_revoke.test because previously 'getuser()' was also a Sentry administrator. Moreover, for some test cases, we had to explicitly alter the owner of a resource in the original grant_revoke.test when we would like to prevent the original owner of the resource, e.g., the creator of the resource, from accessing the resource since the original grant_revoke.test was run without object ownership being taken into consideration. We also note that in this patch we added the decorator of @pytest.mark.execute_serially to each test in test_ranger.py since we have observed that in some cases, e.g., if we are only running the E2E tests in the Jenkins environment, some tests do not seem to be executed sequentially. Testing: - Briefly verified that the implemented statements work as expected in a Kerberized cluster. - Verified that test_ranger.py passes in a local development environment. - Verified that the patch passes the exhaustive tests in the DEBUG build. Change-Id: Ic2b204e62a1d8ae1932d955b4efc28be22202860 Reviewed-on: http://gerrit.cloudera.org:8080/16837 Reviewed-by: Quanlong Huang Tested-by: Impala Public Jenkins --- M bin/create-test-configuration.sh M fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java M fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpaladAuthorizationManager.java M fe/src/main/java/org/apache/impala/authorization/ranger/RangerUtil.java M fe/src/test/java/org/apache/impala/authorization/AuthorizationTestBase.java A
[Impala-ASF-CR] IMPALA-10211 (Part 1): Add support for role-related statements
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16837 ) Change subject: IMPALA-10211 (Part 1): Add support for role-related statements .. Patch Set 13: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/6795/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/16837 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic2b204e62a1d8ae1932d955b4efc28be22202860 Gerrit-Change-Number: 16837 Gerrit-PatchSet: 13 Gerrit-Owner: Fang-Yu Rao Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang Gerrit-Comment-Date: Mon, 21 Dec 2020 08:54:15 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10374: Limit iteration at BufferedTupleStream::DebugString
Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/16884 ) Change subject: IMPALA-10374: Limit iteration at BufferedTupleStream::DebugString .. Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/16884/1/be/src/runtime/buffered-tuple-stream-test.cc File be/src/runtime/buffered-tuple-stream-test.cc: http://gerrit.cloudera.org:8080/#/c/16884/1/be/src/runtime/buffered-tuple-stream-test.cc@1472 PS1, Line 1472: SimpleTupleStreamTest nit: To be consistent with other tests, I think the label should be "SimpleTupleStreamTest::ShortDebugString" http://gerrit.cloudera.org:8080/#/c/16884/1/be/src/runtime/buffered-tuple-stream-test.cc@1485 PS1, Line 1485: // TODO: test that AddRow succeeds after freeing memory. I think we can remove this TODO. http://gerrit.cloudera.org:8080/#/c/16884/1/be/src/runtime/buffered-tuple-stream-test.cc@1508 PS1, Line 1508: 148 Can we calculate 148 out based on the vars? I'm concerning it will introduce flakiness. http://gerrit.cloudera.org:8080/#/c/16884/1/be/src/runtime/buffered-tuple-stream.cc File be/src/runtime/buffered-tuple-stream.cc: http://gerrit.cloudera.org:8080/#/c/16884/1/be/src/runtime/buffered-tuple-stream.cc@189 PS1, Line 189: Comparing to the original codes, we should have "\n" here. -- To view, visit http://gerrit.cloudera.org:8080/16884 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6626c8d54f35f303c01f85be1dd9aa54c8ad9a2d Gerrit-Change-Number: 16884 Gerrit-PatchSet: 1 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Mon, 21 Dec 2020 08:45:38 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6671: Skip locked tables from topic updates
Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/16549 ) Change subject: IMPALA-6671: Skip locked tables from topic updates .. Patch Set 15: (1 comment) http://gerrit.cloudera.org:8080/#/c/16549/15/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java: http://gerrit.cloudera.org:8080/#/c/16549/15/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1325 PS15, Line 1325: LOG.warn("Topic update thread blocking until lock is acquired for table {}", This is too verbose when topicUpdateTblLockMaxWaitTimeMs_ is 0. Almost all tables will leave one log line here because their version <= ctx.toVersion. Could you change the condition to this? if (topicUpdateTblLockMaxWaitTimeMs_ > 0 && !lockWithTimeout) -- To view, visit http://gerrit.cloudera.org:8080/16549 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic657b96edbcdc94c6b906e7ca59291f4e4715655 Gerrit-Change-Number: 16549 Gerrit-PatchSet: 15 Gerrit-Owner: Vihang Karajgaonkar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang Gerrit-Reviewer: Shant Hovsepian Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Mon, 21 Dec 2020 08:12:25 + Gerrit-HasComments: Yes