Jason Fehr has posted comments on this change. ( http://gerrit.cloudera.org:8080/22249 )
Change subject: IMPALA-13201: System Table Queries Execute When Admission Queues are Full ...................................................................... Patch Set 25: (4 comments) http://gerrit.cloudera.org:8080/#/c/22249/24/fe/src/main/java/org/apache/impala/service/Frontend.java File fe/src/main/java/org/apache/impala/service/Frontend.java: http://gerrit.cloudera.org:8080/#/c/22249/24/fe/src/main/java/org/apache/impala/service/Frontend.java@2201 PS24, Line 2201: coordOnlyRequestPool = > nit: use camel case. There have been mixed snake vs camel case style in Fro I prefer camel case since it's Java. I wasn't sure which is correct since I see both (such as enabled_replan in line 2196). I'll switch to camel case though. http://gerrit.cloudera.org:8080/#/c/22249/24/java/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/AllocationFileLoaderService.java File java/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/AllocationFileLoaderService.java: http://gerrit.cloudera.org:8080/#/c/22249/24/java/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/AllocationFileLoaderService.java@83 PS24, Line 83: : // Path to XML file containing allocations. : private File allocFile; : : private Listener reloadListener; : > nit: cleanup the trailing white spaces. Done http://gerrit.cloudera.org:8080/#/c/22249/24/tests/custom_cluster/test_admission_controller.py File tests/custom_cluster/test_admission_controller.py: http://gerrit.cloudera.org:8080/#/c/22249/24/tests/custom_cluster/test_admission_controller.py@1824 PS24, Line 1824: rch(r'\n\s+Executor group 1:\n\s+Verdict: (.*?)\n', profile) : assert fe_verdict, "No fr > I think it is better to make this verbose. Good idea. I modified the code to capture the entire verdict and then do a string compare against the expected verdict. That way, test failures will output the exact different that caused the failure. http://gerrit.cloudera.org:8080/#/c/22249/24/tests/custom_cluster/test_admission_controller.py@1996 PS24, Line 1996: cluster_size=1, > Is this starting a new Coordinator or Executor? Same question for L2007. Both places are adding executors. The @CustomClusterTestSuite annotation starts a cluster consisting of a single coordinator (in the "coordinator" executor group) and no executors. The call to '__run_assert_systables_query' on line 1988 ensures that queries with an only coordinators request pool can still run even if there are no executors. The subsequent calls to '__run_assert_systables_query' ensure queries with only coordinators request pools run just on the coordinators. I updated the comments to better explain what is being added. -- To view, visit http://gerrit.cloudera.org:8080/22249 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5e0e64db92bdbf80f8b5bd85d001ffe4c8c9ffda Gerrit-Change-Number: 22249 Gerrit-PatchSet: 25 Gerrit-Owner: Jason Fehr <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Andrew Sherman <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Comment-Date: Wed, 12 Feb 2025 23:22:29 +0000 Gerrit-HasComments: Yes
