Baymine commented on PR #66312: URL: https://github.com/apache/doris/pull/66312#issuecomment-5191277920
Thanks @morningman — this was a great review, especially point 1. Pushed the changes in `7548f95`. **1. [High] Permanent latch → bounded backoff.** Agreed completely, and I verified the mechanism: `profileIOExecutor` uses `BlockedPolicy` whose `submit()` throws `RejectedExecutionException` after 60s under saturation, which escapes to the load task's `catch` — so the failure was triggered by exactly the scenario this PR targets. And since `writeProfileToStorage()` has no `isProfileLoaded` gate while `deleteBrokenProfiles()` / `deleteOutdatedProfilesFromStorage()` (the only enforcers of `max_spilled_profile_num` / `spilled_profile_storage_limit_bytes`) do, a permanent latch really would trade the thread leak for a disk leak. I took **Option B**: removed `isProfileLoadFailed`, added `consecutiveLoadFailures` + `nextLoadRetryTimeMs` with exponential backoff (1s, 2s, 4s … capped at 5min), giving up only after `MAX_LOAD_RETRY = 10`. The `isProfileLoading` CAS guard remains the actual thread-leak fix, so transient failures now self-heal. **2. [Medium] Bound `waitForProfileLoadFinish()`.** Done — added a `PROFILE_LOAD_WAIT_TIMEOUT_MS = 120s` bound with a WARN on timeout. **4. [Medium] Partial index must not enable cleanup.** Done the minimal way: `loadProfilesFromStorage()` now returns a read-failure count, and only a fully clean load (`readFailures == 0`) calls `markProfileLoaded()`. I also documented the `Profile.read()` boundary you pointed out (it collapses genuinely-malformed files and transient IO errors into the same `null`); making `read()` throw on IO-class errors is the proper fix but is out of scope here — I'll file a follow-up. **5. [Medium] Make the failure test verify "no retry".** Done. It now resets the `pushProfileEntered` probe and asserts a second trigger in the backoff window does **not** re-enter the load body, and asserts the stored profile survives `deleteBrokenProfiles()` + `deleteOutdatedProfilesFromStorage()` after a failed load. **7 / 9. [Low] Logging + `new Thread(...)` cleanup.** Applied the cheap ones: `key=value` log style, demoted the per-batch INFO to DEBUG, and replaced the `new Thread(...) + submit()` anti-pattern with plain lambdas in `deleteBrokenProfiles()` and `writeProfileToStorage()`. **Deferred (happy to do in this PR if you'd prefer):** - **3. [Medium] Single `LoadState` enum + drop the redundant lock** — I like this and it's the cleanest end state, but it churns the ~10 test sites that poke `isProfileLoaded` directly; since backoff already removes the correctness issue I left it as a follow-up. Let me know if you'd rather fold it in here. - **6. [Low] Observability metric** — as you noted, this drops in priority now that backoff logs periodically by construction. - **8. [Low] Test-pool shutdown** — real hygiene point; can add a `@VisibleForTesting shutdown()` if you want it in this PR. FE compiles and `ProfileManagerTest` passes (31 tests, 0 failures/errors). -- 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]
