Copilot commented on code in PR #60574:
URL: https://github.com/apache/doris/pull/60574#discussion_r2773207775
##########
fe/fe-core/src/main/java/org/apache/doris/cloud/rpc/MetaServiceProxy.java:
##########
@@ -262,162 +341,164 @@ public void onFailure(Throwable t) {
}
Review Comment:
The getVisibleVersionAsync method also duplicates the metrics tracking logic
instead of using a consistent pattern. Unlike the other synchronous RPC methods
that now use executeWithMetrics, this async method manually tracks metrics in
multiple places (before the call, in onSuccess, in onFailure, and in the catch
block).
Consider creating an async-specific metrics wrapper method similar to
executeWithMetrics to reduce code duplication and ensure consistent metrics
tracking across both synchronous and asynchronous RPC calls.
##########
fe/fe-core/src/main/java/org/apache/doris/cloud/rpc/MetaServiceProxy.java:
##########
@@ -95,10 +97,28 @@ public boolean needReconn() {
public Cloud.GetInstanceResponse getInstance(Cloud.GetInstanceRequest
request)
throws RpcException {
+ long startTime = System.currentTimeMillis();
+ String methodName = "getInstance";
+ if (MetricRepo.isInit && Config.isCloudMode()) {
+ CloudMetrics.META_SERVICE_RPC_ALL_TOTAL.increase(1L);
+
CloudMetrics.META_SERVICE_RPC_TOTAL.getOrAdd(methodName).increase(1L);
+ }
+
try {
final MetaServiceClient client = getProxy();
- return client.getInstance(request);
+ Cloud.GetInstanceResponse response = client.getInstance(request);
+ if (MetricRepo.isInit && Config.isCloudMode()) {
+ CloudMetrics.META_SERVICE_RPC_LATENCY.getOrAdd(methodName)
+ .update(System.currentTimeMillis() - startTime);
+ }
+ return response;
} catch (Exception e) {
+ if (MetricRepo.isInit && Config.isCloudMode()) {
+ CloudMetrics.META_SERVICE_RPC_ALL_FAILED.increase(1L);
+
CloudMetrics.META_SERVICE_RPC_FAILED.getOrAdd(methodName).increase(1L);
+ CloudMetrics.META_SERVICE_RPC_LATENCY.getOrAdd(methodName)
+ .update(System.currentTimeMillis() - startTime);
+ }
throw new RpcException("", e.getMessage(), e);
}
}
Review Comment:
The getInstance method duplicates the metrics tracking logic that exists in
executeWithMetrics. This method should use executeWithMetrics like other RPC
methods to avoid code duplication and ensure consistent metrics tracking.
Additionally, getInstance bypasses the retry logic in
MetaServiceClientWrapper.executeRequest by directly calling getProxy() and
client.getInstance(request), while all other methods benefit from the retry
mechanism. This inconsistency could lead to different reliability
characteristics for this method compared to others.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
