Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/20762 )
Change subject: IMPALA-12540: Query Live Table ...................................................................... Patch Set 48: (12 comments) http://gerrit.cloudera.org:8080/#/c/20762/47/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: http://gerrit.cloudera.org:8080/#/c/20762/47/be/src/scheduling/admission-controller.cc@1817 PS47, Line 1817: Status AdmissionController::ComputeGroupScheduleStates( Update the doc at admission-controller.h to reflect this changes. http://gerrit.cloudera.org:8080/#/c/20762/47/be/src/scheduling/admission-controller.cc@1864 PS47, Line 1864: all coordinators nit: use dash, "all-coordinators", to match others. I think it will be displayed in query profile. http://gerrit.cloudera.org:8080/#/c/20762/47/be/src/scheduling/scheduler-test-util.h File be/src/scheduling/scheduler-test-util.h: http://gerrit.cloudera.org:8080/#/c/20762/47/be/src/scheduling/scheduler-test-util.h@314 PS47, Line 314: AddCoordinatorScan nit: AddSystemTableScan ? http://gerrit.cloudera.org:8080/#/c/20762/47/be/src/scheduling/scheduler-test-util.cc File be/src/scheduling/scheduler-test-util.cc: http://gerrit.cloudera.org:8080/#/c/20762/47/be/src/scheduling/scheduler-test-util.cc@588 PS47, Line 588: hdfs_file_split.set_length(0); Add DCHECK that this is a system table scan. Is there any similar check needed in the main code? http://gerrit.cloudera.org:8080/#/c/20762/47/be/src/scheduling/scheduler.cc File be/src/scheduling/scheduler.cc: http://gerrit.cloudera.org:8080/#/c/20762/47/be/src/scheduling/scheduler.cc@875 PS47, Line 875: use_coordinator I think this will confuse with coordinator-only query (see L845 to L859). I suggest to rename it to something else, like "is_system_scan". http://gerrit.cloudera.org:8080/#/c/20762/47/be/src/scheduling/scheduler.cc@882 PS47, Line 882: if (coordinatorPB == nullptr) { : // Coordinator is no longer available, skip this range. : LOG(WARNING) << "Coordinator " << coordinator : << " is no longer available for assignment."; : continue; : } Add message in query profile if this situation happen, under SCHEDULER_WARNING_KEY. See L395 for an example. http://gerrit.cloudera.org:8080/#/c/20762/47/be/src/scheduling/scheduler.cc@958 PS47, Line 958: int num_remote_executor_candidates = min(query_options.num_remote_executor_candidates, : executor_config.group.NumExecutors()); unnecessary change? executor_group is executor_config.group in L827. http://gerrit.cloudera.org:8080/#/c/20762/47/be/src/scheduling/scheduler.cc@1431 PS47, Line 1431: "Scheduler assignment to coordinator: " Make it more explicit about system table scan. Maybe, "Scheduler assignment of system table scan to coordinator: ". http://gerrit.cloudera.org:8080/#/c/20762/47/common/thrift/Query.thrift File common/thrift/Query.thrift: http://gerrit.cloudera.org:8080/#/c/20762/47/common/thrift/Query.thrift@1001 PS47, Line 1001: include_coordinators nit: include_all_coordinators ? http://gerrit.cloudera.org:8080/#/c/20762/47/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: http://gerrit.cloudera.org:8080/#/c/20762/47/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@464 PS47, Line 464: includeCoordinatorsInScheduling nit: includeAllCoordinatorsInScheduling ? http://gerrit.cloudera.org:8080/#/c/20762/48/fe/src/main/java/org/apache/impala/service/BackendConfig.java File fe/src/main/java/org/apache/impala/service/BackendConfig.java: http://gerrit.cloudera.org:8080/#/c/20762/48/fe/src/main/java/org/apache/impala/service/BackendConfig.java@487 PS48, Line 487: public void setEnableWorkloadMgmt(boolean enableWorkloadMgmt) Can this be made private/protected and marked with @VisibleForTesting ? http://gerrit.cloudera.org:8080/#/c/20762/47/tests/custom_cluster/test_query_live.py File tests/custom_cluster/test_query_live.py: http://gerrit.cloudera.org:8080/#/c/20762/47/tests/custom_cluster/test_query_live.py@26 PS47, Line 26: : class TestQueryLive(CustomClusterTestSuite): We should add test cases with coordinator restart / shutting down, before and after query admission. I don't mind doing this later in follow-up patch. Might want to file JIRA so we don't forget. -- To view, visit http://gerrit.cloudera.org:8080/20762 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie2f9a449f0e5502078931e7f1c5df6e0b762c743 Gerrit-Change-Number: 20762 Gerrit-PatchSet: 48 Gerrit-Owner: Michael Smith <michael.sm...@cloudera.com> Gerrit-Reviewer: Andrew Sherman <asher...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com> Gerrit-Reviewer: Michael Smith <michael.sm...@cloudera.com> Gerrit-Reviewer: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Comment-Date: Mon, 25 Mar 2024 18:20:56 +0000 Gerrit-HasComments: Yes
