github-actions[bot] commented on code in PR #67636:
URL: https://github.com/apache/doris/pull/67636#discussion_r4044656031
##########
fe/fe-core/src/main/java/org/apache/doris/cloud/datasource/CloudInternalCatalog.java:
##########
@@ -1444,8 +1450,15 @@ private void unprotectUpdateCloudReplica(OlapTable
olapTable, UpdateCloudReplica
LOG.debug("update cloud replica cluster {} replica {} be
{}", info.getClusterId(),
replica.getId(), info.getBeIds().get(i));
((CloudReplica)
replica).updateClusterToPrimaryBe(clusterId, info.getBeIds().get(i));
+ // Routes for dropped compute groups otherwise accumulate
on Followers forever during replay.
+ staleRouteNum += ((CloudReplica)
replica).removeInvalidRoutes();
Review Comment:
[P1] Avoid rescanning every route map for every replayed tablet
One `UpdateCloudReplicaInfo` record can cover all N tablets for one compute
group, and this call scans each replica's full primary and secondary route
maps. Replaying C group records during initial assignment therefore performs N
* (1 + ... + C) backend lookups even when topology has not changed. This runs
on the serial follower replayer and again during checkpoint replay, so a large
catalog can fall behind or stall checkpointing. Please gate or coalesce cleanup
by backend-topology generation (or another topology-removal boundary) instead
of doing it for every route update.
##########
fe/fe-core/src/main/java/org/apache/doris/cloud/datasource/CloudInternalCatalog.java:
##########
@@ -1418,6 +1422,8 @@ private void unprotectUpdateCloudReplica(OlapTable
olapTable, UpdateCloudReplica
}
((CloudReplica) replica).updateClusterToPrimaryBe(clusterId,
info.getBeId());
+ // Routes for dropped compute groups otherwise accumulate on
Followers forever during replay.
+ staleRouteNum += ((CloudReplica)
replica).removeInvalidRoutes();
Review Comment:
[P2] Make replay cleanup atomic with secondary publication
On a readable follower, query routing can call
`updateClusterToSecondaryBe()` while the replayer executes this cleanup.
`removeInvalidRoutes()` can observe no secondary, then the query inserts a live
secondary, and then cleanup removes the unchanged dead primary from the
separate map. Local routing can still use that secondary, but after follower
promotion `getTabletReplicaInfos()` enumerates secondaries through
`getPrimaryComputeGroupIds()`, so the peer candidate is hidden until rebalancer
repair. Please make the per-cluster primary/secondary transition atomic and add
a latch-based test for this interleaving.
##########
fe/fe-core/src/main/java/org/apache/doris/master/Checkpoint.java:
##########
@@ -409,6 +417,48 @@ public ReentrantReadWriteLock getLock() {
return lock;
}
+ @VisibleForTesting
+ static long removeInvalidCloudReplicaRoutes() {
+ if (Config.isNotCloudMode()) {
+ return 0;
+ }
+ long start = System.currentTimeMillis();
+ long removed = 0;
+ // Replay is complete, so Env.getCurrentEnv() resolves to the
checkpoint Env and its backend set.
+ // Take the catalog from the same call removeInvalidRoutes() uses
internally: the replicas being
+ // swept and the backend set deciding staleness must come from one
Env, or a sweep running off the
+ // checkpoint thread would judge this catalog against the serving
cluster's backends.
+ // That Env is private to the checkpoint thread, so the sweep needs no
locks or coordination with
+ // the serving Env, just like postProcessCloudMetadata().
+ Env env = Env.getCurrentEnv();
+ for (Long dbId : env.getInternalCatalog().getDbIds()) {
Review Comment:
[P2] Include recycle-bin replicas in the pre-save sweep
This traversal starts only from `InternalCatalog`, but `CatalogRecycleBin`
separately persists recycled tables and partitions in the same image. If a
route is valid in the base image, replay then drops its backend and recycles
the table or partition, the object is outside this loop and `saveRecycleBin()`
writes the stale route. Verification-time deserialization only cleans the newly
loaded verification Env; it does not rewrite the saved image. Please include
recycled objects through a shared persisted-replica traversal and cover this
replay sequence.
##########
fe/fe-core/src/main/java/org/apache/doris/cloud/catalog/CloudTabletRebalancer.java:
##########
@@ -1018,6 +1018,11 @@ boolean staleRouteSweepNeeded(Set<Long> currentBes) {
pendingSweepRounds = 0;
return false;
}
+ // An empty topology cannot run the replica callback. Keep the last
non-empty baseline so a
+ // newly created compute group triggers the pending sweep.
+ if (currentBes.isEmpty()) {
Review Comment:
[P2] Sweep shadow replicas when consuming the deferred rounds
This branch intentionally preserves the two sweep rounds until a compute
group returns, but `completeRouteInfo()` consumes both through
`loopCloudReplica()`, which visits only `IndexExtState.VISIBLE`. Shadow
replicas are not necessarily route-empty in production: the alter-job daemon
restores its saved cloud context, and inherited task creation calls
`getBackendIdWithoutException()` before promotion, which can publish a route. A
stale route can therefore survive both resumed passes, and promotion does not
schedule another sweep. Please decouple ALL-index cleanup from VISIBLE-only
balancing and test an empty-to-recreated topology with a shadow index.
--
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]
