Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/20762 )
Change subject: IMPALA-12540: Query Live Table ...................................................................... Patch Set 49: (13 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. Done 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 disp It's not displayed in the profile, but I've updated it. As far as I can tell this name won't be used. 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: AddSystemTableScan > nit: AddSystemTableScan ? Done 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: DCHECK(scan_range.has_is_syste > Add DCHECK that this is a system table scan. Done. I didn't find any other assumptions like that, I think this is just a test simplification. 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: okUpExecutorIp( > I think this will confuse with coordinator-only query (see L845 to L859). I Done http://gerrit.cloudera.org:8080/#/c/20762/47/be/src/scheduling/scheduler.cc@882 PS47, Line 882: const TNetworkAddress& coordinator = : host_list[scan_range_locations.locations[0].host_idx]; : const BackendDescriptorPB* coordinatorPB = : executor_config.all_coords.LookUpBackendDesc(FromTNetworkAddress(coordinator)); : if (coordinatorPB == nullptr) { : > Add message in query profile if this situation happen, under SCHEDULER_WARN Done http://gerrit.cloudera.org:8080/#/c/20762/47/be/src/scheduling/scheduler.cc@958 PS47, Line 958: executor_ip = assignment_ctx.SelectExecutorFromCandidates( : executor_candidates, decide_local_ > unnecessary change? executor_group is executor_config.group in L827. Done http://gerrit.cloudera.org:8080/#/c/20762/47/be/src/scheduling/scheduler.cc@1431 PS47, Line 1431: ngeLocationList& scan_range_locations, > Make it more explicit about system table scan. Maybe, "Scheduler assignment Done 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_all_coordina > nit: include_all_coordinators ? Done 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: includeAllCoordinatorsInSchedul > nit: includeAllCoordinatorsInScheduling ? Done 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: @VisibleForTesting > Can this be made private/protected and marked with @VisibleForTesting ? I've marked it VisibleForTesting. All the other methods marked that way are still public, and I get compile errors if I try to change it. 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: from tests.util.workload_management import assert_query : > We should add test cases with coordinator restart / shutting down, before a I've done some. I haven't had a successful test run with retry_on_failure when I restart a coordinator. I think it retries too quickly to allow the restart to complete. http://gerrit.cloudera.org:8080/#/c/20762/49/tests/custom_cluster/test_query_live.py File tests/custom_cluster/test_query_live.py: http://gerrit.cloudera.org:8080/#/c/20762/49/tests/custom_cluster/test_query_live.py@268 PS49, Line 268: result = self.client.execute('select straight_join count(*) from {}.users, ' Needing straight_join here is kind of awkward, but not sure what to do about it. -- 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: 49 Gerrit-Owner: Michael Smith <[email protected]> Gerrit-Reviewer: Andrew Sherman <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Comment-Date: Tue, 26 Mar 2024 00:25:48 +0000 Gerrit-HasComments: Yes
