github-actions[bot] commented on code in PR #66984:
URL: https://github.com/apache/doris/pull/66984#discussion_r3838142297
##########
fe/fe-core/src/main/java/org/apache/doris/cloud/catalog/CloudTabletRebalancer.java:
##########
@@ -1003,9 +1005,37 @@ public void checkDecommissionState(Map<String,
List<Long>> clusterToBes) {
}
}
+ /**
+ * A route can only go stale when a backend disappears, so unless the
backend set lost a member since
+ * the last sweep there is nothing to find. Without this gate the sweep
would walk every replica's
+ * route maps once a second under table.readLock() only to find nothing,
which on a large catalog is
+ * pure allocation. Clearing the snapshot while the switch is off makes
turning it back on sweep once.
+ */
+ @VisibleForTesting
+ boolean staleRouteSweepNeeded(Set<Long> currentBes) {
+ if (!Config.enable_cloud_replica_stale_route_clean) {
+ lastSweptBackends = null;
+ return false;
+ }
+ return lastSweptBackends == null ||
!currentBes.containsAll(lastSweptBackends);
Review Comment:
[P1] Remember additions before checking for their later removal
After a successful sweep at backend set {A}, adding a new compute group with
sole backend B produces {A,B}; this returns false and never advances
lastSweptBackends, although completeRouteInfo() installs and journals groupB ->
B routes on the replicas. When B/groupB is later dropped, the set is {A} again,
which still contains the remembered {A}, so this also returns false. groupB is
then absent from clusterToBes, leaving exactly those newly created stale routes
unswept. Track a topology/removal generation (or otherwise retain additions in
the baseline), and add the missing {A} -> {A,B} -> {A} case.
##########
fe/fe-core/src/main/java/org/apache/doris/cloud/catalog/CloudTabletRebalancer.java:
##########
@@ -1085,7 +1125,12 @@ private boolean completeRouteInfo() {
}
});
- LOG.info("collect to editlog route {} infos, error num {}",
updateReplicaInfos.size(), assignedErrNum[0]);
+ if (sweepStaleRoutes) {
+ markStaleRouteSweepDone(allBes);
Review Comment:
[P1] Close the topology epoch against in-flight route writers
A query can copy/select backend B in hashReplicaToBe(), pause, and publish
it at CloudReplica.java:388-390 only after B/groupB has been removed and this
pass has already swept that replica. This method then marks the B-less topology
complete, so unchanged future rounds never resweep; groupB is no longer a
callback key either. That leaves an invalid route inserted after the one
allowed pass, distinct from the per-map removal race already discussed. Couple
publication and cleanup through a topology/write generation (or force a bounded
retry after pre-drop writers drain), and cover this ordering with a
barrier-based test.
--
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]
