github-actions[bot] commented on code in PR #68833:
URL: https://github.com/apache/doris/pull/68833#discussion_r4229302522
##########
fe/fe-core/src/main/java/org/apache/doris/cloud/catalog/CloudInstanceStatusChecker.java:
##########
@@ -415,6 +417,7 @@ private void diffAndUpdateComputeGroup(Cloud.ClusterPB
cluster, CloudComputeGrou
LOG.info("virtual compute group renamed from {} to {}",
computeGroupNameInFe, clusterNameInMs);
computeGroup.setName(clusterNameInMs);
cloudSystemInfoService.renameVirtualComputeGroup(computeGroup.getId(),
computeGroupNameInFe, computeGroup);
+ lastFailedSyncTimeMap.remove(computeGroupNameInFe);
Review Comment:
[P3] Keep the failure interval attached to the VCG ID. If a VCG is renamed
while the same subgroup is still missing, `checkSubClusters` starts a new timer
under the new name, and this removal discards the old failure age; repeated
renames can defer the warning indefinitely. The name key also lets a newly
created VCG inherit an old same-name timer before obsolete-group cleanup
removes it. Please migrate the timestamp on rename or key it by the stable
cluster ID, and test a rename with an unresolved subgroup.
##########
fe/fe-core/src/main/java/org/apache/doris/cloud/catalog/CloudInstanceStatusChecker.java:
##########
@@ -363,6 +364,7 @@ private void handleExistingVirtualComputeGroup(
return;
}
+ checkSubClusters(clusterInMs.getClusterNamesList(), clusterInMs,
computeClusters);
Review Comment:
[P2] Avoid treating an empty physical subgroup as a failed VCG sync. MS
permits a COMPUTE cluster with zero nodes, but `CloudClusterChecker` registers
its FE name only while adding BEs (and removes it with the last BE). For an
existing VCG with a valid empty standby, this new per-cycle call therefore
keeps the failure timer active and emits `virtual compute err` after the grace
period on every cycle. Please use the MS compute-cluster snapshot to
distinguish an empty but present subgroup from one that is actually absent or
lagging, and cover that case in a test.
##########
fe/fe-core/src/main/java/org/apache/doris/cloud/catalog/CloudInstanceStatusChecker.java:
##########
@@ -517,12 +520,14 @@ private void handleNewVirtualComputeGroup(Cloud.ClusterPB
cluster, List<Cloud.Cl
private void checkSubClusters(List<String> subClusterNames,
Cloud.ClusterPB cluster,
List<Cloud.ClusterPB> computeClustersInPB) {
+ boolean allSubClustersExist = true;
for (String subClusterName : subClusterNames) {
if (cloudSystemInfoService.getCloudClusterIdByName(subClusterName)
== null) {
+ allSubClustersExist = false;
handleFailedSync(cluster, subClusterName, computeClustersInPB);
- continue;
}
- // CloudClusterChecker find sub compute group
+ }
+ if (allSubClustersExist) {
Review Comment:
[P2] Require the subgroup to be a COMPUTE cluster in the current MS snapshot
before clearing this timer. MS can rename physical `p` to `q` without rewriting
an existing VCG's reference to `p`, then reuse `p` as a VCG name. FE stores
virtual names in the same name-to-ID map, so `getCloudClusterIdByName(p)` is
non-null even though no MS COMPUTE `p` exists, and this branch clears the
failure timer indefinitely. The new recovery test also adds its missing name
only to FE. Please compare the referenced MS COMPUTE name and ID with FE before
declaring recovery.
--
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]
