Copilot commented on code in PR #66296:
URL: https://github.com/apache/doris/pull/66296#discussion_r3682012740
##########
fe/fe-core/src/main/java/org/apache/doris/cloud/rpc/VersionHelper.java:
##########
@@ -73,14 +83,16 @@ public static Cloud.GetVersionResponse
getVisibleVersion(Cloud.GetVersionRequest
resp.getStatus(), tryTimes);
}
// sleep random millis, retry rpc failed
- if (tryTimes > Config.metaServiceRpcRetryTimes() / 2) {
- sleepSeveralMs(500, 1000);
- } else {
- sleepSeveralMs(20, 200);
+ if (tryTimes < maxAttempts) {
+ if (tryTimes > maxAttempts / 2) {
+ sleepSeveralMs(500, 1000);
+ } else {
+ sleepSeveralMs(20, 200);
+ }
}
}
- LOG.warn("get version from meta service failed after retry {} times",
tryTimes);
+ LOG.warn("get version from meta service failed after retry {} times",
maxAttempts);
throw new RpcException("get version from meta service", "failed after
retry n times");
}
Review Comment:
The thrown RpcException message is hard-coded as "failed after retry n
times", which makes troubleshooting difficult and is now inconsistent with the
new maxAttempts parameter. Consider including maxAttempts (and ideally the last
status code) in the exception message so callers/logs reflect the real retry
limit used.
##########
fe/fe-core/src/main/java/org/apache/doris/cloud/catalog/CloudSyncVersionDaemon.java:
##########
@@ -58,6 +59,13 @@ protected void runAfterCatalogReady() {
if (!Config.cloud_enable_version_syncer) {
return;
}
+ // This daemon has no ConnectContext, so use the global/default TTLs
to decide whether
+ // the shared version caches need proactive refresh. Finite TTLs
refresh lazily on reads,
+ // while Long.MAX_VALUE never expires and requires this daemon to keep
the cache current.
+ if
(VariableMgr.getDefaultSessionVariable().cloudPartitionVersionCacheTtlMs !=
Long.MAX_VALUE
+ &&
VariableMgr.getDefaultSessionVariable().cloudTableVersionCacheTtlMs !=
Long.MAX_VALUE) {
+ return;
Review Comment:
VariableMgr.getDefaultSessionVariable() is called twice in the same
condition. Storing it in a local variable avoids repeated lookups and ensures
the decision is made against a consistent snapshot if defaults are updated
concurrently.
##########
fe/fe-core/src/test/java/org/apache/doris/cloud/rpc/VersionHelperTest.java:
##########
@@ -0,0 +1,70 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.doris.cloud.rpc;
+
+import org.apache.doris.cloud.proto.Cloud;
+import org.apache.doris.rpc.RpcException;
+
+import org.junit.Assert;
+import org.junit.Test;
+import org.mockito.MockedStatic;
+import org.mockito.Mockito;
+
+import java.util.concurrent.CompletableFuture;
+
+public class VersionHelperTest {
+ @Test
+ public void testGetVisibleVersionUsesSpecifiedMaxAttempts() throws
RpcException {
+ Cloud.GetVersionRequest request =
Cloud.GetVersionRequest.newBuilder().build();
+ Cloud.GetVersionResponse failedResponse =
Cloud.GetVersionResponse.newBuilder()
+ .setStatus(Cloud.MetaServiceResponseStatus.newBuilder()
+ .setCode(Cloud.MetaServiceCode.KV_TXN_GET_ERR))
+ .build();
+ MetaServiceProxy proxy = Mockito.mock(MetaServiceProxy.class);
+ Mockito.when(proxy.getVisibleVersionAsync(request))
+ .thenReturn(CompletableFuture.completedFuture(failedResponse));
+
+ try (MockedStatic<MetaServiceProxy> mockedProxy =
Mockito.mockStatic(MetaServiceProxy.class)) {
+ mockedProxy.when(MetaServiceProxy::getInstance).thenReturn(proxy);
+
+ Assert.assertThrows(RpcException.class, () ->
VersionHelper.getVisibleVersion(request, 3));
+ }
+
+ Mockito.verify(proxy,
Mockito.times(3)).getVisibleVersionAsync(request);
+ }
Review Comment:
This test triggers VersionHelper's real retry backoff sleeps (20–200ms then
500–1000ms), which can add ~0.5–1.2s to the unit test runtime and introduce
randomness. You can still validate the "uses maxAttempts" behavior with
maxAttempts=2, which keeps the test fast (only the short sleep happens once)
while preserving coverage of the retry loop.
--
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]
