morningman commented on PR #66312:
URL: https://github.com/apache/doris/pull/66312#issuecomment-5162228826
## 1. [High] Replace the permanent terminal flag with bounded backoff, or
drop `isProfileLoadFailed` entirely
**Why this matters.**
The `profileIOExecutor` pool is created with `BlockedPolicy(poolName, 60)`
(`ThreadPoolManager.java:140-145`). When the pool saturates, `submit()` blocks
for up to 60 seconds and then throws `RejectedExecutionException`. In
`loadProfilesFromStorage()`, the submit call is not wrapped in a try/catch:
```java
// ProfileManager.java:694-701
for (String profileDirAbsPath : batch) {
profileIOFutures.add(profileIOExecutor.submit(() -> { // can throw
RejectedExecutionException
Profile profile = Profile.read(profileDirAbsPath);
...
}));
}
```
So the exception escapes to the outer `catch (Exception)` in `loadTask`,
`loadSucceeded` stays `false`, and `isProfileLoadFailed` is latched
permanently. In other words, **the saturation scenario this PR sets out to fix
is exactly the scenario that trips the "never retry" branch.**
From that point on, for the lifetime of the FE process:
- `isProfileLoaded` stays `false`;
- `deleteBrokenProfiles()` (`ProfileManager.java:953`) and
`deleteOutdatedProfilesFromStorage()` (`ProfileManager.java:857`) both return
early — permanently no-ops;
- but `writeProfileToStorage()` (called at `ProfileManager.java:559`) has
**no** `isProfileLoaded` gate and keeps writing new profiles to disk every tick.
The net effect is that `Config.max_spilled_profile_num` and
`Config.spilled_profile_storage_limit_bytes` — enforced only in
`getProfilesToBeRemoved()` (`ProfileManager.java:840-841`) — stop being
enforced until the FE restarts, and the profile directory grows without bound.
Before this PR the retry loop leaked threads, but it also meant a transient
failure could self-heal.
The key observation: **the `isProfileLoading` CAS guard already fixes the
thread leak completely.** With it, at most one loader exists at any moment;
even if every load fails, the result is one short-lived thread per tick that
exits normally — no accumulation. `isProfileLoadFailed` adds nothing to the
leak fix and contributes only the regression risk above.
**Option A (simplest — worth evaluating first).** Drop `isProfileLoadFailed`
and keep only the CAS guard. Failures retry once per tick, serially, with no
leak, and the system self-heals. The cost is one ERROR log per second while
failing, which a failure counter plus rate-limited logging solves.
**Option B (conservative).** Bounded exponential backoff, preserving
self-healing:
```java
private final AtomicLong nextRetryTimeMs = new AtomicLong(0);
private final AtomicInteger consecutiveFailures = new AtomicInteger(0);
private static final int MAX_LOAD_RETRY = 10;
// entry
if (checkIfProfileLoaded() || consecutiveFailures.get() >= MAX_LOAD_RETRY
|| System.currentTimeMillis() < nextRetryTimeMs.get()) {
return;
}
// failure branch in finally
int failures = consecutiveFailures.incrementAndGet();
// 1s, 2s, 4s ... capped at 5 minutes
long backoffMs = Math.min(1000L << Math.min(failures, 8), 300_000L);
nextRetryTimeMs.set(System.currentTimeMillis() + backoffMs);
LOG.warn("Profile cold load failed, attempt={}, nextRetryInMs={},
loadedProfileCount={}",
failures, backoffMs, queryIdToProfileMap.size());
```
Either way, please spell out the blast radius in the PR description. It
currently says only "will not be retried until the FE restarts" and does not
mention that this also disables disk quota enforcement.
## 2. [Medium] Bound the wait in `waitForProfileLoadFinish()`
```java
// ProfileManager.java:729-739
private void waitForProfileLoadFinish() {
while (isProfileLoading.get() && !checkIfProfileLoaded()) {
Thread.sleep(100);
...
}
}
```
There is no upper bound. If the in-flight loader is stuck in `submit()` (up
to 60 s per call under `BlockedPolicy`) or in `future.get()` (unbounded), a
synchronous caller blocks indefinitely. The old `loadThread.join()` was equally
unbounded, so this is not a regression — but it is precisely the scenario this
PR targets, so a bound belongs here.
Note that `sync == true` currently has no production caller (a repo-wide
grep finds only `ProfileManager.java:558` passing `false`; every other call
site is in the test file), so there is no live impact today. The method is
`protected` though, and the synchronous contract is part of the API surface.
```java
private void waitForProfileLoadFinish(long timeoutMs) {
long deadline = System.currentTimeMillis() + timeoutMs;
while (isProfileLoading.get() && !checkIfProfileLoaded()) {
if (System.currentTimeMillis() >= deadline) {
LOG.warn("Timed out waiting for in-flight profile load,
timeoutMs={}", timeoutMs);
return;
}
try {
Thread.sleep(100);
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
LOG.warn("Interrupted while waiting for profile loading to
finish", e);
return;
}
}
}
```
## 3. [Medium] Collapse the three flags into a single state machine
`isProfileLoaded` (a `volatile boolean` additionally guarded by a read-write
lock), `isProfileLoading`, and `isProfileLoadFailed` together encode one state
machine using two different synchronization mechanisms, leaving the reader to
work out which flag combinations are legal. Collapsing them into an enum
reduces the entry's three-step "pre-check + CAS + recheck" dance to a single
CAS:
```java
private enum LoadState { NOT_LOADED, LOADING, LOADED, FAILED }
private final AtomicReference<LoadState> loadState = new
AtomicReference<>(LoadState.NOT_LOADED);
protected void loadProfilesFromStorageIfFirstTime(boolean sync) {
if (!loadState.compareAndSet(LoadState.NOT_LOADED, LoadState.LOADING)) {
if (sync && loadState.get() == LoadState.LOADING) {
waitForProfileLoadFinish(WAIT_TIMEOUT_MS);
}
return;
}
...
}
```
A single CAS needs no recheck, so the explanatory comment at
`ProfileManager.java:620-623` disappears along with the code it justifies, and
illegal states are ruled out at the type level.
This also lets you remove `isProfileLoadedLock`: guarding a single `volatile
boolean` with a `ReentrantReadWriteLock` is redundant (a pre-existing issue),
and the comment at `ProfileManager.java:126-127` — "no further write operation,
so no data race" — is now stale given the new `LOADING`/`FAILED` states.
One caveat: `isProfileLoaded` is package-private and read/written directly
by tests, so the refactor needs matching test updates.
## 4. [Medium] Close the gap in "a partial index must not enable destructive
cleanup"
The intent of the second commit is right, but `Profile.read()` swallows
every exception:
```java
// Profile.java, read(String path)
} catch (Exception exception) {
LOG.error("read profile failed", exception);
return null;
}
```
A transient read failure — EIO, a permission problem, a file caught
half-written by a concurrent `writeProfileToStorage()` — therefore yields
`null`, the profile is silently skipped, `future.get()` does not throw, and
`loadSucceeded` remains `true`. `markProfileLoaded()` runs, and on the next
tick `deleteBrokenProfiles()` deletes those still-valid files.
So the invariant the comments promise holds only for exceptions that escape
to the outer `try`. Because both `Profile.read()` and
`getOnStorageProfileInfos()` have their own catch-all handlers, that escape
path is narrow — in practice only a throwing `pushProfile()` and a rejected
`submit()`, and the new test has to override `pushProfile` to reach it at all.
The minimal improvement is to count read failures and feed them into the
decision:
```java
int readFailures = 0;
for (Future<?> future : profileIOFutures) {
try {
future.get();
} catch (Exception e) {
readFailures++;
LOG.warn("Failed to read profile from storage", e);
}
}
```
The deeper problem is that `Profile.read()` collapses "the file is genuinely
malformed" (should be deleted) and "a transient IO error occurred" (must not be
deleted) into the same `null`. The proper fix is for `Profile.read()` to throw
on IO-class exceptions and return `null` only for format errors, so callers can
distinguish them. That is beyond this PR's scope and could be a follow-up issue
— but please at least document the current boundary in a comment, so future
readers do not assume `isProfileLoaded == true` implies a complete index.
## 5. [Medium] Make the failure test actually verify "no retry"
`testLoadProfilesFromStorageFailure` currently asserts, on the second
invocation:
```java
pm.loadProfilesFromStorageIfFirstTime(true);
Assertions.assertFalse(pm.isProfileLoaded);
```
This assertion holds even if `isProfileLoadFailed` is deleted entirely — the
second load would simply fail again and leave the flag `false`. The test does
not exercise the property it is meant to protect. Suggested rewrite:
```java
// A second trigger must not enter the load body at all
pushProfileEntered.set(false);
pm.loadProfilesFromStorageIfFirstTime(true);
Assertions.assertFalse(pushProfileEntered.get(),
"a terminated load must not be restarted by a later trigger");
Assertions.assertFalse(pm.isProfileLoaded);
```
Please also assert the invariant the second commit really cares about — that
stored files survive a failed load:
```java
File[] filesBefore = tempDir.listFiles();
Assertions.assertEquals(1, filesBefore.length);
pm.deleteBrokenProfiles();
pm.deleteOutdatedProfilesFromStorage();
Assertions.assertEquals(1, tempDir.listFiles().length,
"a failed load must not enable destructive cleanup of valid stored
profiles");
```
## 6. [Low] Add observability for the terminal-failure state
The WARN at `ProfileManager.java:649` fires exactly once. An operator who
misses that single line has no way to discover that profile disk cleanup is
permanently disabled on that FE. Consider exposing a gauge metric (e.g.
`doris_fe_profile_load_state`), or re-emitting the warning at a low rate (say
every 10 minutes) from `runAfterCatalogReady()`.
If suggestion 1 is adopted, this drops in priority — backoff retries log
periodically by construction.
## 7. [Low] Two small logging adjustments
The repo's `AGENTS.md` has no Logging Standards section, so these follow
general conventions and the surrounding code:
- `ProfileManager.java:649`: `Loaded profile count in memory: {}` does not
follow the `key=value` style used elsewhere. Prefer `loadedProfileCount={}`.
- `ProfileManager.java:638` (ERROR) and `:649` (WARN) describe the same
event at two different levels. Merging them into one record — exception plus
the "will not retry" conclusion together — makes alert rules easier to write.
Separately, `ProfileManager.java:716` (`"Processed batch {} - {} of {}
profiles"`) fires once per 10 profiles, since `BATCH_SIZE = 10`
(`ProfileManager.java:77`). With 5000 stored profiles, that is 500 INFO lines
on every FE start. This is pre-existing, but since the method was extracted
anyway, demoting it to DEBUG or replacing it with a single summary after the
load completes is a cheap win.
## 8. [Low] `ProfileManager` instances and thread pools in tests
The five new `new ProfileManager()` instances (the file had one before this
PR) each construct a `fetch-realtime-profile-pool` (10 threads) and a
`profile-io-thread-pool` (≥20 threads) that are never shut down — roughly 150
extra live threads in the FE unit-test JVM. `ThreadPoolManager`'s class javadoc
also states that "the thread pool name in fe must be unique", and duplicate
instances overwrite each other's entries in `nameToThreadPoolMap`. Harmless in
tests, but worth tightening. Consider adding a `@VisibleForTesting shutdown()`
to `ProfileManager` and calling it from `@AfterEach`.
## 9. [Low] Clean up the remaining `new Thread(...)` + `submit()`
anti-pattern
This PR already fixed this pattern on the cold-load path (constructing a
`Thread` object purely to use it as a `Runnable`), but two instances remain in
the same file:
- `deleteBrokenProfiles()`, `ProfileManager.java:961`
- `writeProfileToStorage()`, inside the `for (ProfileElement profileElement
: profilesToBeStored)` loop
Switching them to plain lambdas is a zero-risk consistency cleanup, in line
with the `AGENTS.md` guidance to follow existing similar code in similar
contexts. Whether to fold this into the current PR or a follow-up is the
author's call.
---
## Additional Test Gaps
Beyond suggestion 5, the following are uncovered:
- The `loadThread.start()` failure path (`ProfileManager.java:664-673`).
Genuinely hard to construct — acceptable to skip.
- The recheck-after-CAS path (`ProfileManager.java:624-627`).
- `testWaitForProfileLoadInterrupted` does not assert that the interrupt
flag was preserved, even though the code restores it via
`Thread.currentThread().interrupt()`.
- No test confirms the loader thread is actually a daemon.
---
## Summary
The direction is right: the CAS guard genuinely fixes the thread leak
described in issue #66311, and both the code organization and the test quality
are above average.
**Suggestion 1 is worth resolving before merge.** The permanent
`isProfileLoadFailed` latch is triggered by exactly the thread-pool-saturation
scenario this PR targets, and once latched, profile disk quotas stop being
enforced until the FE restarts — trading a thread leak for a disk leak. Since
the flag contributes nothing to the thread-leak fix, removing it or converting
it to bounded backoff is a small change.
Suggestions 2 through 5 are worth handling in this PR; the rest are optional.
--
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]
