Yida Wu has posted comments on this change. ( http://gerrit.cloudera.org:8080/23760 )
Change subject: IMPALA-14612: Add global metrics for admission state map size ...................................................................... Patch Set 2: (8 comments) Thanks Jason for the review. http://gerrit.cloudera.org:8080/#/c/23760/1/be/src/scheduling/admission-control-service.h File be/src/scheduling/admission-control-service.h: http://gerrit.cloudera.org:8080/#/c/23760/1/be/src/scheduling/admission-control-service.h@134 PS1, Line 134: /// Tracks the memory usage of payload in the service queue. > Since admission_state_map_size_ is initialized in the class constructor, th Use another way to implement. http://gerrit.cloudera.org:8080/#/c/23760/1/be/src/scheduling/admission-control-service.cc File be/src/scheduling/admission-control-service.cc: http://gerrit.cloudera.org:8080/#/c/23760/1/be/src/scheduling/admission-control-service.cc@157 PS1, Line 157: VLOG(1) << "AdmitQuery: query_id=" << req->query_id() > In this situation, since we know only 1 query is being added, it would be b I would prefer to avoid maintaining a separate state that requires manual updates in every Add/Remove path. The intent here is to observe the actual size of the map to catch potential leaks, reading the map size directly seems more reliable. Implemented a FunctionGauge approach instead. http://gerrit.cloudera.org:8080/#/c/23760/1/be/src/scheduling/admission-control-service.cc@228 PS1, Line 228: TRuntimeProfileTree tree; > In this situation, since we know only 1 query is being removed, it would be Use another way to implement. http://gerrit.cloudera.org:8080/#/c/23760/1/be/src/scheduling/admission-control-service.cc@253 PS1, Line 253: RespondAndReleaseRpc(status, resp, rpc_context); > In this situation, since we know only 1 query is being removed, it would be Use another way to implement. http://gerrit.cloudera.org:8080/#/c/23760/1/be/src/scheduling/admission-control-service.cc@318 PS1, Line 318: AdmissionHeartbeatResponsePB* resp, kudu::rpc::RpcContext* rpc_context) { > Can the above for loop directly call admission_state_map_size_->Increment(- Use another way to implement. http://gerrit.cloudera.org:8080/#/c/23760/1/be/src/scheduling/admission-control-service.cc@337 PS1, Line 337: discard_result(admission_state_map_.Delete(query_id)); > Can the above for loop directly call admission_state_map_size_->Increment(- Use another way to implement. http://gerrit.cloudera.org:8080/#/c/23760/1/tests/custom_cluster/test_admission_controller.py File tests/custom_cluster/test_admission_controller.py: http://gerrit.cloudera.org:8080/#/c/23760/1/tests/custom_cluster/test_admission_controller.py@2392 PS1, Line 2392: client2 = coord2.service.create_hs2_client() > Can this assert be made more accurate? Possibly assert old_admission_state Done http://gerrit.cloudera.org:8080/#/c/23760/1/tests/custom_cluster/test_admission_controller.py@2423 PS1, Line 2423: @SkipIfNotHdfsMinicluster.tuned_for_minicluster > Instead of sleeping, use self.cluster.admissiond().service.wait_for_metric_ Done -- To view, visit http://gerrit.cloudera.org:8080/23760 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie803aabf8d91b6381c5d0d7534cd9c9fc2166a73 Gerrit-Change-Number: 23760 Gerrit-PatchSet: 2 Gerrit-Owner: Yida Wu <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Yida Wu <[email protected]> Gerrit-Comment-Date: Fri, 12 Dec 2025 11:21:34 +0000 Gerrit-HasComments: Yes
