github-actions[bot] commented on code in PR #68833:
URL: https://github.com/apache/doris/pull/68833#discussion_r4236128743
##########
fe/fe-core/src/main/java/org/apache/doris/cloud/catalog/CloudInstanceStatusChecker.java:
##########
@@ -516,32 +522,44 @@ private void handleNewVirtualComputeGroup(Cloud.ClusterPB
cluster, List<Cloud.Cl
}
private void checkSubClusters(List<String> subClusterNames,
Cloud.ClusterPB cluster,
- List<Cloud.ClusterPB> computeClustersInPB) {
+ Map<String, Cloud.ClusterPB>
computeClustersByName) {
+ boolean allSubClustersExist = true;
for (String subClusterName : subClusterNames) {
- if (cloudSystemInfoService.getCloudClusterIdByName(subClusterName)
== null) {
- handleFailedSync(cluster, subClusterName, computeClustersInPB);
- continue;
+ Cloud.ClusterPB subClusterInMs =
computeClustersByName.get(subClusterName);
+ String subClusterIdInFe =
cloudSystemInfoService.getCloudClusterIdByName(subClusterName);
Review Comment:
[P3] Compare both subgroups against one FE name-map snapshot.
`getCloudClusterIdByName` reads each name separately while CloudClusterChecker
renames physical groups one ID at a time. With an MS snapshot containing `p/P`
and `q/Q`, this loop can read `p/P`, then see FE rename `p` away and map `q` to
`Q`, and finally read `q/Q`. It clears the VCG failure timer even though FE
never had both mappings at once; the next cycle restarts the grace period. Copy
the FE name map under its read lock before checking the subgroup set.
##########
fe/fe-core/src/test/java/org/apache/doris/cloud/catalog/CloudInstanceStatusCheckerTest.java:
##########
@@ -112,6 +113,100 @@ public void testSyncInstanceCreatesVirtualComputeGroup() {
Assertions.assertEquals("standby_cg",
virtualComputeGroup.getStandbyComputeGroup());
}
+ @Test
+ public void testVirtualComputeGroupKeepsFailureUntilAllSubGroupsExist() {
+ addComputeGroup("standby_cg_id", "standby_cg");
+
Mockito.doReturn(instanceResponseWithVirtualComputeGroup()).when(cloudSystemInfoService).getCloudInstance();
+
+ CloudInstanceStatusChecker checker = new
CloudInstanceStatusChecker(cloudSystemInfoService);
+ checker.runAfterCatalogReady();
+
+ Assertions.assertTrue(failedSyncTimes(checker).containsKey("vcg_id"));
+ }
+
+ @Test
+ public void testVirtualComputeGroupAcceptsEmptySubGroups() {
+
Mockito.doReturn(instanceResponseWithEmptyComputeGroups()).when(cloudSystemInfoService).getCloudInstance();
+
+ CloudInstanceStatusChecker checker = new
CloudInstanceStatusChecker(cloudSystemInfoService);
+ checker.runAfterCatalogReady();
+
+ Assertions.assertTrue(failedSyncTimes(checker).isEmpty());
+ }
+
+ @Test
+ public void testVirtualComputeGroupRejectsMismatchedFeSubGroupId() {
+ addComputeGroup("wrong_active_cg_id", "active_cg");
+ addComputeGroup("standby_cg_id", "standby_cg");
+
Mockito.doReturn(instanceResponseWithVirtualComputeGroup()).when(cloudSystemInfoService).getCloudInstance();
+
+ CloudInstanceStatusChecker checker = new
CloudInstanceStatusChecker(cloudSystemInfoService);
+ checker.runAfterCatalogReady();
+
+ Assertions.assertTrue(failedSyncTimes(checker).containsKey("vcg_id"));
+ }
+
+ @Test
+ public void
testExistingVirtualComputeGroupDoesNotRecoverFromFeOnlySubGroup() {
+ addComputeGroup("active_cg_id", "active_cg");
+ addComputeGroup("standby_cg_id", "standby_cg");
+ Cloud.GetInstanceResponse missingActiveResponse =
+ instanceResponseWithVirtualComputeGroup("missing_active_cg",
"standby_cg");
+ Cloud.GetInstanceResponse recoveredActiveResponse =
instanceResponseWithVirtualComputeGroup(
+ "vcg", "missing_active_cg", "standby_cg",
+ computeGroup("missing_active_cg_id", "missing_active_cg"),
+ computeGroup("standby_cg_id", "standby_cg"));
+ Mockito.doReturn(instanceResponseWithVirtualComputeGroup())
+ .doReturn(missingActiveResponse)
+ .doReturn(missingActiveResponse)
+ .doReturn(recoveredActiveResponse)
+ .when(cloudSystemInfoService).getCloudInstance();
+
+ CloudInstanceStatusChecker checker = new
CloudInstanceStatusChecker(cloudSystemInfoService);
+ checker.runAfterCatalogReady();
+ Assertions.assertTrue(failedSyncTimes(checker).isEmpty());
+
+ checker.runAfterCatalogReady();
+ Assertions.assertTrue(failedSyncTimes(checker).containsKey("vcg_id"));
+
+ addComputeGroup("missing_active_cg_id", "missing_active_cg");
+ checker.runAfterCatalogReady();
+ Assertions.assertTrue(failedSyncTimes(checker).containsKey("vcg_id"));
+
+ checker.runAfterCatalogReady();
+ Assertions.assertTrue(failedSyncTimes(checker).isEmpty());
+ }
+
+ @Test
+ public void testDropInvalidVirtualComputeGroupClearsFailure() {
+ Mockito.doReturn(instanceResponseWithVirtualComputeGroup())
+ .doReturn(instanceResponseWithoutVirtualComputeGroup())
+ .when(cloudSystemInfoService).getCloudInstance();
+
+ CloudInstanceStatusChecker checker = new
CloudInstanceStatusChecker(cloudSystemInfoService);
+ checker.runAfterCatalogReady();
+ Assertions.assertTrue(failedSyncTimes(checker).containsKey("vcg_id"));
+
+ checker.runAfterCatalogReady();
+
Assertions.assertNull(cloudSystemInfoService.getComputeGroupById("vcg_id"));
+ Assertions.assertTrue(failedSyncTimes(checker).isEmpty());
+ }
+
+ @Test
+ public void testRenameVirtualComputeGroupKeepsFailureTime() {
+ Mockito.doReturn(instanceResponseWithVirtualComputeGroup("old_vcg",
"active_cg", "standby_cg"))
+ .doReturn(instanceResponseWithVirtualComputeGroup("new_vcg",
"active_cg", "standby_cg"))
+ .when(cloudSystemInfoService).getCloudInstance();
+
+ CloudInstanceStatusChecker checker = new
CloudInstanceStatusChecker(cloudSystemInfoService);
+ checker.runAfterCatalogReady();
+ Long failedSyncTime = failedSyncTimes(checker).get("vcg_id");
+ Assertions.assertNotNull(failedSyncTime);
+
+ checker.runAfterCatalogReady();
+ Assertions.assertEquals(failedSyncTime,
failedSyncTimes(checker).get("vcg_id"));
Review Comment:
[P3] Make the rename timer assertion independent of wall-clock resolution.
Both checker runs happen back to back, and a regression that clears and
recreates the `vcg_id` entry during rename can assign the same
`System.currentTimeMillis()` value, so this equality can pass even though the
failure age was lost. Seed the map entry with a deliberately old value before
the second run and assert that exact value remains.
--
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]
