Gabriel39 commented on issue #66497: URL: https://github.com/apache/doris/issues/66497#issuecomment-5214852990
Thanks for the v4 revision. The release scope is now substantially aligned with the confirmed 4.2 boundary: branch-4.1, the pinned one-shot lance-c APIs, no distributed/incremental build dependency, the existing `vector_search()` query contract, and a separate REST capability matrix. I do not think the design needs another structural rewrite. There are still four issues that should be corrected before approval. ## 1. A job must not become `CANCELLED` while its external mutation can still commit The proposed behavior persists a fence, marks the job `CANCELLED`, stops observing and refreshing, while the one-shot native operation may continue and commit. This makes the terminal state false: a CREATE, REPLACE, or even DROP can be reported as cancelled although its side effect succeeded. It also leaves Doris metadata stale and can release an admission slot while the BE is still consuming resources. For 4.2, please use one of these semantics: - allow cancellation only while the job is `PENDING`, before dispatch, and reject CANCEL once the one-shot call is `RUNNING`; this is my preferred option; or - keep a running job in `CANCEL_REQUESTED`/`CANCELLING`, continue observing it, and select the terminal state from the real outcome. It must not enter `CANCELLED` until non-execution/non-commit is known. If the operation may have committed, metadata refresh must not be deliberately suppressed. ## 2. The persisted job record is missing the identity required to recover a Directory task The v4 job record does not contain the selected BE, BE task ID, dispatch attempt/generation, or any queryable executor identity. Nevertheless, the failover section says a new FE resumes polling or refresh. Consider: FE1 dispatches the one-shot call to BE3 and fails while BE3 is still running. FE2 can replay a record saying only `RUNNING`; it does not know which BE/task to query, and the Directory path has neither a Namespace transaction nor a lance-c job-status API. It therefore cannot resume polling. Please either: - define a Doris-layer durable/queryable BE task protocol and persist at least `selected_be_id`, `be_task_id`, and dispatch generation before dispatch; or - state that loss of FE/BE task communication during a running Directory operation transitions the job to `OUTCOME_UNKNOWN` and is not recoverable by polling in 4.2. This is also necessary for correct admission accounting after FE failover. ## 3. A returned error is not always proof of a pre-commit failure The current classification treats any BE/REST call that returns an error as a confirmed pre-commit failure. That is only safe for errors that prove the operation never committed, such as local validation failures, unsupported operations, definite not-found/already-exists outcomes, or a definite commit conflict. RPC timeout/disconnect, object-store I/O around commit, BE process loss, and REST timeout/5xx/lost response may have an unknown outcome. Please define an error/outcome matrix based on typed error classes: - proven not dispatched/not committed → `FAILED`; - proven committed → refresh and finish; - transport loss, timeout, ambiguous commit-stage I/O, or lost executor state → `OUTCOME_UNKNOWN` plus refresh-on-observation. Do not classify all non-zero C returns or REST failures as confirmed pre-commit failures without an API guarantee for each error class. ## 4. IVF_PQ defaults do not match the actual lance-c 0.1.2 implementation The retained v3 property semantics make `num_partitions` and `num_sub_vectors` optional, but the pinned implementation calls `require_field` for both: `build_ivf` requires `num_partitions > 0`, and `build_pq` requires `num_sub_vectors > 0`. https://github.com/lance-format/lance-c/blob/v0.1.2/src/index.rs#L315-L357 Passing zero therefore fails even though the header comments suggest an internal default. The 4.2 contract must either require both properties for IVF_PQ or have Doris fill concrete, documented values. I prefer requiring them in the initial version rather than introducing an implicit data-dependent default policy. ## Additional details to tighten - `SHOW INDEX.Properties` should not recover metric/build parameters from a historical job record. The Lance manifest/Namespace is authoritative, the current job record does not establish that its definition still corresponds to the current UUID/version, and an external writer may have replaced the index. If 0.1.2 cannot read a property back, leave it empty rather than expose potentially stale auxiliary state. - Define how `OUTCOME_UNKNOWN` jobs are acknowledged/resolved or eventually retained/expired; otherwise they remain permanently visible even after an operator has converged the external state. - For REST polling, define status-to-state mapping, polling interval/timeout, handling before an index first appears, failure states, and separate CREATE/DROP completion conditions. Namespace 0.7.7 exposes `status` as a string rather than a normative enum. - Make the `file://` restriction enforceable. "Identical contents on every FE/BE" cannot be verified generically; rejecting local mutation in multi-node deployments and allowing it only in an explicitly supported single-node/test configuration would be clearer. - Specify where Directory metadata reads for `SHOW INDEX`, `IF EXISTS`, and reconciliation execute: FE Java SDK or a synchronous BE metadata RPC using `index_count`/`index_list_json`. The mutation-task design alone does not define this path. Once the four primary issues are corrected, v4 is close to an approvable 4.2 design. -- 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]
