Gabriel39 commented on PR #67754: URL: https://github.com/apache/doris/pull/67754#issuecomment-5887127563
Reviewed head `ef2684b61b76e49f8c82172d66a227f9aed94596`. I found two correctness issues and a retention-capacity risk. ### 1. GC can remove a job that still owns a possible-live worker slot [`removeResolvedJobsOlderThan()`](https://github.com/apache/doris/blob/ef2684b61b76e49f8c82172d66a227f9aed94596/fe/fe-core/src/main/java/org/apache/doris/datasource/lance/job/LanceIndexJobManager.java#L590-L617) checks only `!job.isUnresolved()` and the retention age. However, [`isUnresolved()` and `holdsPossibleLiveSlot()`](https://github.com/apache/doris/blob/ef2684b61b76e49f8c82172d66a227f9aed94596/fe/fe-core/src/main/java/org/apache/doris/datasource/lance/job/LanceIndexJob.java#L315-L354) represent different obligations. A job can receive a trusted success result, become COMMITTED, and finish its metadata refresh while still lacking a matching child-exit proof. It then has `isUnresolved() == false` but `holdsPossibleLiveSlot() == true`. Once the retention window expires, this GC deletes the record and therefore loses the outstanding worker-slot accounting and termination tracking. Retention expiry is not proof of worker termination, and the retention setting can also be reduced below its seven-day default. Please require both `!job.isUnresolved()` and `!job.holdsPossibleLiveSlot()` before removing a record. Explicitly force-released jobs remain eligible because force release already makes `holdsPossibleLiveSlot()` false. Add coverage showing that a terminal, refresh-complete job without termination proof survives GC, then becomes eligible after the matching proof arrives. This is also relevant to #67978: fixing its concurrency accounting to include possible-live jobs would still be undermined if GC deletes those records. The current slice has no real worker, so this describes the lifecycle/integration defect rather than an observed resource failure in the default configuration. ### 2. A concurrent termination proof makes FORCE_RELEASE incorrectly report that the job is not UNKNOWN The command reads a job revision, performs the external metadata read and refresh, then calls `forceRelease()` with that original revision. During the external work, [`recordTerminationProof()`](https://github.com/apache/doris/blob/ef2684b61b76e49f8c82172d66a227f9aed94596/fe/fe-core/src/main/java/org/apache/doris/datasource/lance/job/LanceIndexJobManager.java#L408-L448) can record CHILD_REAPED or BE_PROCESS_EPOCH_GONE and increment the revision **without changing UNKNOWN**. The release CAS then fails, but [the command maps every unsuccessful reread that is not already force-released to error 5104](https://github.com/apache/doris/blob/ef2684b61b76e49f8c82172d66a227f9aed94596/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/ResolveLanceIndexJobCommand.java#L183-L194). The operator sees "not in UNKNOWN state" while SHOW still reports UNKNOWN and the fence/quota remain held. Please distinguish a state change from a revision-only update. If the job remains UNKNOWN, use a bounded retry with the appropriate revalidation or return an explicit concurrent-update/retry error instead of 5104. Add a command test that records termination proof between the initial read and the release CAS. ### 3. The fixed per-round removal cap can make the retention backlog grow indefinitely `LanceIndexJobCleaner` removes at most 1024 records per round and defaults to one round per hour: at most 24,576 records per day. If the sustained completion rate exceeds that, expired records accumulate indefinitely. The global unresolved-job quota does not bound completed historical records. The manager also gathers and sorts all expired records while holding its write lock, before truncating to 1024. A growing backlog therefore increases both FE memory use and the time other job operations wait for the lock. Consider treating 1024 as a per-journal-batch limit, draining multiple batches within a bounded round budget or scheduling an earlier continuation when a batch is full. Candidate selection should also avoid sorting the entire expired backlog under the write lock. This is a capacity risk under sustained volume, not a claim that ordinary low-volume workloads already exhaust memory. Separately, FORCE_RELEASE remains UNKNOWN-only and cannot unblock permanently undispatchable PENDING jobs; the head-of-line limitation discussed on #67978 needs a separate solution. This review was based on code inspection; I did not run builds or tests. -- 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]
