Gabriel39 commented on PR #67978: URL: https://github.com/apache/doris/pull/67978#issuecomment-5886754180
Reviewed head `7b965c32ef2270733827d074d56ae1397e4184ee`. I found two issues to address and one local-file topology restriction to align with the accepted design. ### 1. Per-BE concurrency accounting excludes workers that may still be alive [`countInflightByBackend()`](https://github.com/apache/doris/blob/7b965c32ef2270733827d074d56ae1397e4184ee/fe/fe-core/src/main/java/org/apache/doris/datasource/lance/job/LanceIndexJobDispatcher.java#L293-L298) counts only `RUNNING` jobs. However, deadline expiry or an ambiguous send failure can transition a job to `UNKNOWN` while its possible-live slot remains held. For example, with a per-BE limit of 1: dispatch A, let A time out while its worker continues running, then run another dispatch round. A no longer contributes to the count, so B can be dispatched to the same BE. The configured limit therefore does not bound potentially live workers, which can undermine resource limits once the real worker is connected. Please account for `holdsPossibleLiveSlot()` and cover both cases: an UNKNOWN job continues to consume capacity, and a matching termination proof releases it. This also needs a safe release path for a trusted **not-enqueued** rejection: [`completePreInvocationRejected()`](https://github.com/apache/doris/blob/7b965c32ef2270733827d074d56ae1397e4184ee/fe/fe-core/src/main/java/org/apache/doris/datasource/lance/job/LanceIndexJobDispatcher.java#L490-L499) currently leaves the possible-live marker set, so changing the counting predicate alone would strand capacity after the current BE stub rejects a request. The mutation gate is disabled by default and this PR has no real worker, so this is not a claim of an OOM in the current default configuration. ### 2. Restoring the regression-test interval does not resume the dispatcher promptly The dispatch suite [sets the polling interval to 3600 seconds](https://github.com/apache/doris/blob/7b965c32ef2270733827d074d56ae1397e4184ee/regression-test/suites/external_table_p0/lance/test_lance_index_dispatch.groovy#L147-L158) and restores the configuration in `finally`. The admission suite now uses the same technique. However, [`Daemon.run()`](https://github.com/apache/doris/blob/7b965c32ef2270733827d074d56ae1397e4184ee/fe/fe-core/src/main/java/org/apache/doris/common/util/Daemon.java#L117-L128) calls `Thread.sleep(intervalMs)`, and the dispatcher reads the configuration only at the start of its next round. Restoring the configuration cannot interrupt the existing one-hour sleep. The configuration looks restored while dispatch, deadline/epoch sweeps, and refresh retries can remain paused for almost an hour, affecting subsequent tests on the same cluster. Please use a pause/resume mechanism that actually resumes the daemon, or make interval updates wake it safely. The test should verify that polling resumes after cleanup. ### 3. The local-file guard accepts a multi-BE cluster when only one BE is alive [`isOnlyAliveBackend()`](https://github.com/apache/doris/blob/7b965c32ef2270733827d074d56ae1397e4184ee/fe/fe-core/src/main/java/org/apache/doris/datasource/lance/job/LanceIndexJobDispatcher.java#L514-L516) uses `getAllBackendIds(true)`. A deployment with one FE and two registered BEs passes this guard when one BE loses its heartbeat. That is weaker than the [accepted exactly-one-FE-and-BE restriction](https://github.com/apache/doris/issues/66497#issuecomment-5301826623); heartbeat loss does not establish a single-node deployment or shared local-file identity. Please check the registered topology and add a test with two registered BEs but only one alive. This becomes an execution-safety concern when the real local-file worker is enabled; the current stub does not mutate data. Separately, the documented head-of-line blocking remains an accepted limitation, and isolated-worker resource enforcement and real end-to-end fault tests remain necessary before enabling the feature. 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]
