github-actions[bot] commented on code in PR #67636:
URL: https://github.com/apache/doris/pull/67636#discussion_r4061226492
##########
fe/fe-core/src/main/java/org/apache/doris/cloud/catalog/CloudTabletRebalancer.java:
##########
@@ -1029,24 +1035,62 @@ boolean staleRouteSweepNeeded(Set<Long> currentBes) {
// baseline. Advancing only after a sweep would leave the baseline at
the pre-addition set, and
// dropping that same backend later would compare equal to it and go
unnoticed.
lastSweptBackends = currentBes;
- if (pendingSweepRounds > 0) {
+ // Outside the configured window, leave pendingSweepRounds untouched
rather than draining it: a
+ // backend that goes away outside the window must still get its two
rounds once the window
+ // opens, not lose them to rounds that never actually swept.
+ if (pendingSweepRounds > 0 && isStaleRouteCleanTimeAllowed()) {
pendingSweepRounds--;
Review Comment:
[P2] Schedule pending cleanup into the configured window
Leaving `pendingSweepRounds` intact is not enough to guarantee a sweep: the
daemon's next wake-up is still driven only by the independently mutable
`cloud_tablet_rebalancer_interval_second`. If that cadence is longer than, or
phase-shifted away from, a short window (for example a daily/long interval with
a ten-minute window), every run can miss the window and the pending cleanup
remains unboundedly deferred. Please schedule the next pending round at the
next window opening, or validate/enforce an interval that cannot skip the
window, and add a scheduler-level test.
##########
fe/fe-core/src/main/java/org/apache/doris/cloud/datasource/CloudInternalCatalog.java:
##########
@@ -1390,6 +1392,83 @@ public void
replayUpdateCloudReplica(UpdateCloudReplicaInfo info) throws MetaNot
}
}
+ // The three private helpers below are each independently callable (one
per catalog level), so this
+ // guard is checked in every one of them rather than once at the top -- a
config flip mid-sweep then
+ // takes effect at the next db/table/partition boundary instead of only on
the next whole-catalog call.
+ private static boolean routeCleanupDisabled() {
+ return Config.isNotCloudMode() ||
!Config.enable_cloud_replica_stale_route_clean
+ || FeConstants.runningUnitTest;
+ }
+
+ /**
+ * @param systemInfo the backend set to judge staleness against. Must come
from the same Env this
+ * catalog belongs to (the serving Env during
replay/rebalancing, or the checkpoint's
+ * private Env while generating an image) -- never
resolved internally via
+ * Env.getCurrentSystemInfo(), so a caller cannot
accidentally sweep this catalog's
+ * replicas against a different Env's backend set.
+ */
+ public long removeInvalidCloudReplicaRoutes(SystemInfoService systemInfo) {
+ if (routeCleanupDisabled()) {
+ return 0;
+ }
+ long start = System.currentTimeMillis();
+ long removed = 0;
+ for (Long dbId : getDbIds()) {
+ Database db = getDbNullable(dbId);
+ if (db == null) {
+ continue; // The database can be dropped concurrently on the
serving Env.
+ }
+ removed += removeInvalidCloudReplicaRoutes(db, systemInfo);
+ }
+ LOG.info("swept stale cloud routes, entries dropped {}, cost {} ms",
+ removed, System.currentTimeMillis() - start);
+ return removed;
+ }
+
+ private static long removeInvalidCloudReplicaRoutes(Database db,
SystemInfoService systemInfo) {
+ if (routeCleanupDisabled()) {
+ return 0;
Review Comment:
[P2] Do not acknowledge a sweep aborted by a config flip
These nested guards make a mid-scan disable return normally after only part
of the catalog, but `CloudEnv.replayJournal()` cannot distinguish that from a
complete pass and advances `cleanedBackendRemovalVersion`. If the mutable flag
is re-enabled before the next replay call observes the disabled state, the
removal version is unchanged and the skipped databases are never retried.
Please snapshot the enabled state for an indivisible pass, or return an
explicit incomplete result and advance the generation only after full
traversal; a latch-based disable/re-enable test across database boundaries
would cover the race.
##########
fe/fe-core/src/main/java/org/apache/doris/master/Checkpoint.java:
##########
@@ -159,6 +161,13 @@ public synchronized void doCheckpoint() throws
CheckpointException {
}
env.postProcessAfterMetadataReplayed(false);
postProcessCloudMetadata();
+ try {
+ removeInvalidCloudReplicaRoutes(env);
+ } catch (Exception e) {
Review Comment:
[P2] Sweep every replica graph written to the image
This only walks `InternalCatalog`, but later image modules write independent
graphs containing `CloudReplica`: retained `BackupJob`s can hold a `backupMeta`
table deep-copy, and retained `RollupJobV2`s keep `partitionIdToRollupIndex`
even after cancellation detaches the index or the job finishes. If either graph
captures a route while backend B exists and a later drop is replayed, this pass
cannot remove B before `backupHandler` or `alterJob` serializes it. The `db`
and `alterJob` modules also deserialize independently, so active-job copies are
not alias-cleaned after a load. Verification loading may sanitize its temporary
Env, but cannot rewrite the image already emitted. Please sweep every persisted
replica owner (or strip these caches) and test backup and cancelled-rollup
states.
##########
fe/fe-core/src/main/java/org/apache/doris/cloud/catalog/CloudTabletRebalancer.java:
##########
@@ -1029,24 +1035,62 @@ boolean staleRouteSweepNeeded(Set<Long> currentBes) {
// baseline. Advancing only after a sweep would leave the baseline at
the pre-addition set, and
// dropping that same backend later would compare equal to it and go
unnoticed.
lastSweptBackends = currentBes;
- if (pendingSweepRounds > 0) {
+ // Outside the configured window, leave pendingSweepRounds untouched
rather than draining it: a
+ // backend that goes away outside the window must still get its two
rounds once the window
+ // opens, not lose them to rounds that never actually swept.
+ if (pendingSweepRounds > 0 && isStaleRouteCleanTimeAllowed()) {
pendingSweepRounds--;
return true;
}
return false;
}
+ /**
+ * Whether the configured cleanup window
(cloud_tablet_rebalancer_stale_route_clean_start_time to
+ * ..._end_time) contains the current time. Equal start/end -- including
the "00:00"/"00:00" default --
+ * means unrestricted, matching the pre-existing behavior of sweeping
whenever staleRouteSweepNeeded()
+ * says a sweep is due. An unparseable configuration also falls back to
unrestricted rather than
+ * silently disabling cleanup.
+ */
+ @VisibleForTesting
+ boolean isStaleRouteCleanTimeAllowed() {
+ LocalTime start =
parseCleanTime(Config.cloud_tablet_rebalancer_stale_route_clean_start_time);
+ LocalTime end =
parseCleanTime(Config.cloud_tablet_rebalancer_stale_route_clean_end_time);
Review Comment:
[P2] Make cleanup-window reconfiguration atomic
The start and end are separate mutable configs, but this method immediately
interprets any observed pair as a complete window. SQL can update only one
config per statement, and the REST handler also applies entries sequentially.
For example, changing `23:00-23:30` to `13:00-13:30` end-first temporarily
creates the wrapping window `23:00-13:30`; a pending one-second daemon round at
noon can consume a full-catalog sweep outside both the old and intended
maintenance windows. Please publish/validate the pair atomically (or reject
mixed generations) and add a test that attempts a sweep between the endpoint
updates.
##########
fe/fe-core/src/main/java/org/apache/doris/cloud/catalog/CloudTabletRebalancer.java:
##########
@@ -1029,24 +1035,62 @@ boolean staleRouteSweepNeeded(Set<Long> currentBes) {
// baseline. Advancing only after a sweep would leave the baseline at
the pre-addition set, and
// dropping that same backend later would compare equal to it and go
unnoticed.
lastSweptBackends = currentBes;
- if (pendingSweepRounds > 0) {
+ // Outside the configured window, leave pendingSweepRounds untouched
rather than draining it: a
+ // backend that goes away outside the window must still get its two
rounds once the window
+ // opens, not lose them to rounds that never actually swept.
+ if (pendingSweepRounds > 0 && isStaleRouteCleanTimeAllowed()) {
pendingSweepRounds--;
return true;
}
return false;
}
+ /**
+ * Whether the configured cleanup window
(cloud_tablet_rebalancer_stale_route_clean_start_time to
+ * ..._end_time) contains the current time. Equal start/end -- including
the "00:00"/"00:00" default --
+ * means unrestricted, matching the pre-existing behavior of sweeping
whenever staleRouteSweepNeeded()
+ * says a sweep is due. An unparseable configuration also falls back to
unrestricted rather than
+ * silently disabling cleanup.
+ */
+ @VisibleForTesting
+ boolean isStaleRouteCleanTimeAllowed() {
+ LocalTime start =
parseCleanTime(Config.cloud_tablet_rebalancer_stale_route_clean_start_time);
+ LocalTime end =
parseCleanTime(Config.cloud_tablet_rebalancer_stale_route_clean_end_time);
+ if (start == null || end == null || start.equals(end)) {
+ return true;
+ }
+ return isWithinWindow(LocalTime.now(TimeUtils.getDorisZoneId()),
start, end);
+ }
+
+ @VisibleForTesting
+ static boolean isWithinWindow(LocalTime now, LocalTime start, LocalTime
end) {
+ if (start.isBefore(end)) {
+ return !now.isBefore(start) && !now.isAfter(end);
+ }
+ // across midnight, e.g. 23:00 - 06:00
+ return !now.isBefore(start) || !now.isAfter(end);
+ }
+
+ private static LocalTime parseCleanTime(String time) {
+ try {
+ return LocalTime.parse(time, STALE_ROUTE_CLEAN_TIME_FORMAT);
+ } catch (DateTimeParseException e) {
+ LOG.warn("invalid cloud_tablet_rebalancer_stale_route_clean time:
{}", time);
+ return null;
+ }
+ }
+
private boolean completeRouteInfo() {
List<UpdateCloudReplicaInfo> updateReplicaInfos = new
ArrayList<UpdateCloudReplicaInfo>();
long[] assignedErrNum = {0L};
- long[] staleRouteNum = {0L};
boolean sweepStaleRoutes = staleRouteSweepNeeded(allBes);
- // loopCloudReplica() has the compute group loop innermost, so it
hands us every replica once per
- // live compute group, while removeInvalidRoutes() scans the whole
route map and does not care
- // which group we are on. Pin the sweep to one arbitrary group id so a
sweeping round still makes a
- // single pass per replica. If clusterToBes is empty the callback
never runs at all, so the serving
- // catalog keeps the entries until it reloads the image -- there is
nothing to route in that state.
- String sweepTicket = sweepStaleRoutes ?
clusterToBes.keySet().stream().findFirst().orElse(null) : null;
+ // Cleanup is independent of compute groups and includes shadow
indices of active tables.
+ // Take the catalog and backend set from the same Env so the sweep can
never mix the two.
+ Env currentEnv = Env.getCurrentEnv();
+ long staleRouteNum = sweepStaleRoutes
+ ? ((CloudInternalCatalog) currentEnv.getInternalCatalog())
+
.removeInvalidCloudReplicaRoutes(currentEnv.getClusterInfo())
Review Comment:
[P2] Run stale-route cleanup when multi-replica mode skips balancing
This sweep is only reached through `completeRouteInfo()`, but
`runAfterCatalogReady()` returns before it whenever the mutable
`enable_cloud_multi_replica` flag is set. A concrete sequence is to populate
route maps in normal mode, enable multi-replica, and then drop a compute group:
the live master does not replay its own `OP_DROP_BACKEND`, and the
multi-replica query branch returns from rendezvous hashing without touching
these legacy maps, so their dropped-backend entries remain until restart or the
flag is disabled. Please move the cleanup trigger ahead of (or independent
from) the balancing-only early return and cover this transition.
--
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]
