github-actions[bot] commented on code in PR #67913:
URL: https://github.com/apache/doris/pull/67913#discussion_r3999699948
##########
fe/fe-core/src/main/java/org/apache/doris/cloud/system/CloudSystemInfoService.java:
##########
@@ -250,7 +250,7 @@ public void addVirtualClusterInfoToMapsNoLock(String
clusterId, String clusterNa
public void removeVirtualClusterInfoFromMapsNoLock(String clusterId,
String clusterName) {
LOG.info("remove virtual cluster info from maps, clusterId={},
clusterName={}", clusterId, clusterName);
clusterIdToBackend.remove(clusterId);
- clusterNameToId.remove(clusterName);
+ clusterNameToId.remove(clusterName, clusterId);
Review Comment:
[P1] Apply the expected-ID guard to rename cleanup too
This fixes stale group deletion, but `updateClusterNameToId` and
`renameVirtualClusterInfoFromMapsNoLock` still remove the old name without
checking its owner. A valid sequence is rename `A: X -> Y`, then create `B: X`.
The physical checker adds B first (`X -> B`) and later reconciles A with
`updateClusterNameToId(Y, X, A)`, whose unconditional `remove(X)` deletes B's
current mapping; subsequent cycles see both IDs and never restore it. Please
use `remove(oldName, clusterId)` in both rename helpers and add
rename-plus-name-reuse coverage, including the virtual analogue.
##########
fe/fe-core/src/test/java/org/apache/doris/cloud/system/CloudSystemInfoServiceTest.java:
##########
@@ -1062,6 +1062,49 @@ public void testContainsCloudCluster() {
Assertions.assertFalse(infoService.containsCloudCluster("cluster_2"));
}
+ @Test
+ public void testRemovingRecreatedComputeGroupKeepsCurrentNameMapping() {
+ infoService = new CloudSystemInfoService();
+ String clusterName = "recreated_cluster";
+ String oldClusterId = "old_cluster_id";
+ String newClusterId = "new_cluster_id";
+
+ Backend oldBackend = new Backend(1L, "127.0.0.1", 9050);
+ Map<String, String> oldTagMap = Tag.DEFAULT_BACKEND_TAG.toMap();
+ oldTagMap.put(Tag.CLOUD_CLUSTER_NAME, clusterName);
+ oldTagMap.put(Tag.CLOUD_CLUSTER_ID, oldClusterId);
+ oldBackend.setTagMap(oldTagMap);
+ infoService.updateCloudClusterMapNoLock(List.of(oldBackend), new
ArrayList<>());
+
+ Backend newBackend = new Backend(2L, "127.0.0.2", 9050);
+ Map<String, String> newTagMap = Tag.DEFAULT_BACKEND_TAG.toMap();
+ newTagMap.put(Tag.CLOUD_CLUSTER_NAME, clusterName);
+ newTagMap.put(Tag.CLOUD_CLUSTER_ID, newClusterId);
+ newBackend.setTagMap(newTagMap);
+ infoService.updateCloudClusterMapNoLock(List.of(newBackend), new
ArrayList<>());
+
+ infoService.updateCloudClusterMapNoLock(new ArrayList<>(),
List.of(oldBackend));
+
+ Assertions.assertEquals(newClusterId,
infoService.getCloudClusterIdByName(clusterName));
Review Comment:
[P1] Preserve ownership across stale daemon snapshots
This assertion also assumes one current, sequential view. The instance and
physical cluster checkers fetch Meta Service independently: an instance
response containing virtual `V:X` can pause, the physical checker can install
current physical `P:X`, and the stale response can then overwrite `X -> P` with
`X -> V`. On the next current instance cycle, `remove(X, V)` deletes the only
mapping; P still exists by ID with backend tags already named X, so later
physical cycles never rebuild `X -> P`. Expected-ID deletion cannot help once
the stale add became the apparent owner. Please make name reconciliation
generation-aware or explicitly repair every current ID-to-name association, and
add a latch/barrier interleaving test plus a later repair cycle.
##########
fe/fe-core/src/test/java/org/apache/doris/cloud/system/CloudSystemInfoServiceTest.java:
##########
@@ -1062,6 +1062,49 @@ public void testContainsCloudCluster() {
Assertions.assertFalse(infoService.containsCloudCluster("cluster_2"));
}
+ @Test
+ public void testRemovingRecreatedComputeGroupKeepsCurrentNameMapping() {
+ infoService = new CloudSystemInfoService();
+ String clusterName = "recreated_cluster";
+ String oldClusterId = "old_cluster_id";
+ String newClusterId = "new_cluster_id";
+
+ Backend oldBackend = new Backend(1L, "127.0.0.1", 9050);
+ Map<String, String> oldTagMap = Tag.DEFAULT_BACKEND_TAG.toMap();
+ oldTagMap.put(Tag.CLOUD_CLUSTER_NAME, clusterName);
+ oldTagMap.put(Tag.CLOUD_CLUSTER_ID, oldClusterId);
+ oldBackend.setTagMap(oldTagMap);
+ infoService.updateCloudClusterMapNoLock(List.of(oldBackend), new
ArrayList<>());
+
+ Backend newBackend = new Backend(2L, "127.0.0.2", 9050);
+ Map<String, String> newTagMap = Tag.DEFAULT_BACKEND_TAG.toMap();
+ newTagMap.put(Tag.CLOUD_CLUSTER_NAME, clusterName);
+ newTagMap.put(Tag.CLOUD_CLUSTER_ID, newClusterId);
+ newBackend.setTagMap(newTagMap);
+ infoService.updateCloudClusterMapNoLock(List.of(newBackend), new
ArrayList<>());
+
+ infoService.updateCloudClusterMapNoLock(new ArrayList<>(),
List.of(oldBackend));
Review Comment:
[P1] Exercise and fix the production reconciliation path
This direct delete bypasses `CloudClusterChecker.checkToDelCluster`. In a
real cycle, starting with `name -> oldId` and remote state containing only
`(newId, name)` on a distinct endpoint, `checkToAddCluster` first overwrites
the mapping with `name -> newId`. `checkToDelCluster` then calls
`getClusterNameByClusterId(oldId)`, gets an empty string, and returns before
deleting the old backends/group. The same skip repeats every cycle, so the
patched helper is never reached and stale old IDs accumulate. Please make
obsolete-cluster cleanup ID-driven and cover the checker entry path, including
old-ID removal and a second reconciliation cycle.
--
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]
