github-actions[bot] commented on code in PR #66984:
URL: https://github.com/apache/doris/pull/66984#discussion_r3821675727
##########
fe/fe-core/src/main/java/org/apache/doris/cloud/catalog/CloudReplica.java:
##########
@@ -669,5 +697,10 @@ public void gsonPostProcess() throws IOException {
}
this.primaryClusterToBackends = null;
}
+ // outside the `bes` branch on purpose: the new `be` format
accumulates stale entries just the same.
+ // The backends module is loaded before db/recycleBin
(PersistMetaModules.MODULE_NAMES), and the
+ // checkpoint thread resolves Env.getCurrentEnv() to its own Env, so
the backend set read here is
+ // the one belonging to the image being loaded.
+ removeInvalidRoutes();
Review Comment:
[P1] Clean routes after checkpoint journal replay
`gsonPostProcess()` runs during `env.loadImage()`, but checkpoint creation
then calls `env.replayJournal(checkPointVersion)` before `saveImage()`. If
route updates and the corresponding backend drops are newer than the last
successful image, this hook either never sees those routes or sees their BEs as
still present; replay then removes the BEs, and the stale routes are serialized
into the new image. Once that image reaches the finalized journal ID, no second
checkpoint is guaranteed, so this can preserve the exact post-image buildup
that the fix needs to recover from. Please run one exhaustive route sweep after
replay and before save across every serialized route-bearing object (including
the recycle bin), and cover load -> route/drop replay -> save in a lifecycle
test.
##########
fe/fe-core/src/main/java/org/apache/doris/cloud/catalog/CloudReplica.java:
##########
@@ -596,6 +597,33 @@ public void clearClusterToBe(String cluster) {
secondaryClusterToBackends.remove(cluster);
}
+ /**
+ * Drop the route entries whose backend has been dropped from the cluster.
+ *
+ * Such an entry is already dead weight: getBackendIdImpl() resolves the
backend id, gets null and
+ * falls back to hashReplicaToBe(), so removing it does not change
routing. But nothing ever removes
+ * it either -- dropCluster() only touches CloudSystemInfoService, and the
rebalancer only walks the
+ * compute groups that currently exist -- so entries of dropped compute
groups pile up forever, both
+ * in FE heap and in the image (the `bes`/`be` field).
+ *
+ * @return how many entries were dropped
+ */
+ public int removeInvalidRoutes() {
+ if (!Config.enable_cloud_replica_stale_route_clean ||
FeConstants.runningUnitTest) {
+ return 0;
+ }
+ SystemInfoService systemInfo = Env.getCurrentSystemInfo();
+ int before = primaryClusterToBackend.size() +
secondaryClusterToBackends.size();
+ secondaryClusterToBackends.entrySet().removeIf(e ->
systemInfo.getBackend(e.getValue().key()) == null);
+ // Keep a dead primary whose compute group still has a live secondary.
With
+ // enable_immediate_be_assign=false that is the normal failover state,
and the lazy fetch path in
+ // FrontendServiceImpl.getTabletReplicaInfos() enumerates secondaries
through the primary key set,
+ // so dropping the key would hide a live secondary from the peer cache
candidates.
+ primaryClusterToBackend.entrySet().removeIf(e ->
systemInfo.getBackend(e.getValue()) == null
Review Comment:
[P2] Revalidate the secondary atomically before removing its primary key
The two maps are updated concurrently outside an exclusive table lock. A
query can add a live secondary after this predicate observes `containsKey ==
false` but before `removeIf` removes the dead primary, leaving a live secondary
with no primary key. `getTabletReplicaInfos()` enumerates secondaries only
through `getPrimaryComputeGroupIds()`, so that cache candidate is hidden until
a later callback repairs the key. Please make the per-group primary/secondary
decision atomic (or revalidate with a representation that cannot publish a
secondary-only state) and add a barrier-based interleaving test.
##########
fe/fe-core/src/main/java/org/apache/doris/cloud/catalog/CloudReplica.java:
##########
@@ -596,6 +597,33 @@ public void clearClusterToBe(String cluster) {
secondaryClusterToBackends.remove(cluster);
}
+ /**
+ * Drop the route entries whose backend has been dropped from the cluster.
+ *
+ * Such an entry is already dead weight: getBackendIdImpl() resolves the
backend id, gets null and
+ * falls back to hashReplicaToBe(), so removing it does not change
routing. But nothing ever removes
+ * it either -- dropCluster() only touches CloudSystemInfoService, and the
rebalancer only walks the
+ * compute groups that currently exist -- so entries of dropped compute
groups pile up forever, both
+ * in FE heap and in the image (the `bes`/`be` field).
+ *
+ * @return how many entries were dropped
+ */
+ public int removeInvalidRoutes() {
+ if (!Config.enable_cloud_replica_stale_route_clean ||
FeConstants.runningUnitTest) {
+ return 0;
+ }
+ SystemInfoService systemInfo = Env.getCurrentSystemInfo();
+ int before = primaryClusterToBackend.size() +
secondaryClusterToBackends.size();
+ secondaryClusterToBackends.entrySet().removeIf(e ->
systemInfo.getBackend(e.getValue().key()) == null);
+ // Keep a dead primary whose compute group still has a live secondary.
With
+ // enable_immediate_be_assign=false that is the normal failover state,
and the lazy fetch path in
+ // FrontendServiceImpl.getTabletReplicaInfos() enumerates secondaries
through the primary key set,
+ // so dropping the key would hide a live secondary from the peer cache
candidates.
+ primaryClusterToBackend.entrySet().removeIf(e ->
systemInfo.getBackend(e.getValue()) == null
+ && !secondaryClusterToBackends.containsKey(e.getKey()));
+ return before - primaryClusterToBackend.size() -
secondaryClusterToBackends.size();
Review Comment:
[P3] Count successful removals instead of subtracting concurrent sizes
Route writers mutate both maps concurrently, so these three independent size
samples are not a valid removal count. A secondary added after `before` can
make this return negative, and additions can also cancel actual removals in the
INFO aggregate. Please increment the count only when a conditional remove
succeeds (for example `remove(key, expectedValue)`) and cover a concurrent
writer with a barrier-based test.
##########
fe/fe-core/src/main/java/org/apache/doris/cloud/catalog/CloudTabletRebalancer.java:
##########
@@ -1017,6 +1027,15 @@ private boolean completeRouteInfo() {
for (Tablet tablet : tablets) {
for (Replica r : tablet.getReplicas()) {
CloudReplica replica = (CloudReplica) r;
+ // Drop routes of compute groups that no longer exist;
gsonPostProcess() only converges
+ // the catalog on image load, so without this the leader
keeps them until it restarts.
+ // No edit log op is written for the removal: the entries
are already unroutable, every
Review Comment:
[P1] Add a convergence boundary for follower route state
This removal is neither journaled nor replayed, and `CloudTabletRebalancer`
is started only by `startMasterOnlyDaemonThreads()`. A follower that loads a
route and later replays that backend's drop keeps the stale route for its
entire lifetime; pushed checkpoint images are only downloaded by `/put`, not
loaded into the serving Env. It can therefore retain the same heap leak and
carry it through promotion (where zero-group/multi-replica states still skip
the sweep), contrary to the claim that every FE reaches the same conclusion.
Please clean after serving journal replay on every FE or persist/replay an
equivalent bounded cleanup, and test follower replay plus promotion without
restart.
##########
fe/fe-core/src/main/java/org/apache/doris/cloud/catalog/CloudTabletRebalancer.java:
##########
@@ -1017,6 +1027,15 @@ private boolean completeRouteInfo() {
for (Tablet tablet : tablets) {
for (Replica r : tablet.getReplicas()) {
CloudReplica replica = (CloudReplica) r;
+ // Drop routes of compute groups that no longer exist;
gsonPostProcess() only converges
+ // the catalog on image load, so without this the leader
keeps them until it restarts.
+ // No edit log op is written for the removal: the entries
are already unroutable, every
+ // FE reaches the same conclusion from its own backend
set, and the image is written by
+ // the checkpoint Env from a fresh load, so a
leader/follower difference never reaches
+ // persisted state and is gone after one round on the new
leader.
+ if (cluster.equals(cleanupPassTicket)) {
+ staleRouteNum[0] += replica.removeInvalidRoutes();
Review Comment:
[P1] Sweep every retained route-bearing metadata owner
This pass visits only live visible indexes. Normal drops detach
tables/partitions into `CatalogRecycleBin`; finished `RollupJobV2` records
retain serialized installed-index graphs that become separate copies after
image load; and warmup can populate routes on shadow indexes that
`IndexExtState.VISIBLE` skips. Backend drops therefore leave these maps stale
for their retention/lifecycle windows even while normal leader balancing
continues. Please define a bounded visitor over every retained route-bearing
owner and reuse it for topology/post-replay cleanup, with recycle/recover,
finished-rollup image, and warmed-shadow coverage.
##########
fe/fe-core/src/main/java/org/apache/doris/cloud/catalog/CloudTabletRebalancer.java:
##########
@@ -1006,6 +1006,16 @@ public void checkDecommissionState(Map<String,
List<Long>> clusterToBes) {
private boolean completeRouteInfo() {
List<UpdateCloudReplicaInfo> updateReplicaInfos = new
ArrayList<UpdateCloudReplicaInfo>();
long[] assignedErrNum = {0L};
+ long[] staleRouteNum = {0L};
+ // removeInvalidRoutes() scans a replica's whole route map, which has
nothing to do with the compute
+ // group currently being processed. But loopCloudReplica() has the
compute group loop innermost, so
+ // it hands us every replica once per live compute group -- calling
the cleanup unconditionally
+ // would repeat the same scan N times per replica, all of it under
table.readLock(). Any single
Review Comment:
[P1] Do not rescan every replica route map every second
The ticket prevents N scans per live group, but this still performs two full
map traversals and backend lookups once per replica on every default one-second
round, all while `loopCloudReplica()` holds the table's fair read lock. After
the first cleanup, that O(replicas * retained routes) work is entirely
redundant and directly lengthens metadata-writer waits. Please gate or
rate-limit the sweep on a backend-topology generation (and gather replica
references without holding the lock for the expensive scan), with scale
coverage that bounds lock hold time.
##########
fe/fe-core/src/main/java/org/apache/doris/cloud/catalog/CloudTabletRebalancer.java:
##########
@@ -1006,6 +1006,16 @@ public void checkDecommissionState(Map<String,
List<Long>> clusterToBes) {
private boolean completeRouteInfo() {
List<UpdateCloudReplicaInfo> updateReplicaInfos = new
ArrayList<UpdateCloudReplicaInfo>();
long[] assignedErrNum = {0L};
+ long[] staleRouteNum = {0L};
+ // removeInvalidRoutes() scans a replica's whole route map, which has
nothing to do with the compute
+ // group currently being processed. But loopCloudReplica() has the
compute group loop innermost, so
+ // it hands us every replica once per live compute group -- calling
the cleanup unconditionally
+ // would repeat the same scan N times per replica, all of it under
table.readLock(). Any single
+ // compute group id works as a ticket to run it exactly once per
replica per round; which one is
+ // irrelevant. If clusterToBes is empty the callback is never invoked
at all, so the serving
+ // catalog keeps the stale entries until it reloads the image -- there
is nothing to route in that
+ // state anyway.
+ String cleanupPassTicket =
clusterToBes.keySet().stream().findFirst().orElse(null);
Review Comment:
[P1] Make serving cleanup independent of a live-group callback
When `clusterToBes` is empty, `loopCloudReplica()` never invokes this
callback, so after the last compute group is dropped every stale per-replica
route remains in the serving FE indefinitely. The same sweep is bypassed
entirely by the earlier `enable_cloud_multi_replica` return, even though that
setting is mutable and old single-replica routes can remain allocated. 'Nothing
to route' does not address this PR's heap/image-leak goal. Please run a
replica-only cleanup boundary even with zero groups and before the
multi-replica balancing early return, with integration coverage for both states.
--
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]
