Gabriel39 commented on issue #66497: URL: https://github.com/apache/doris/issues/66497#issuecomment-5226393984
Thanks for the v4.1 delta. It correctly addresses the four primary round-4 issues: cancellation is now pre-dispatch only, Directory failover no longer claims resumable polling without executor identity, ambiguous errors are separated from proven no-commit failures, and the IVF_PQ properties match the pinned lance-c implementation. I do not think the design is approval-ready yet. Three correctness issues remain. ## 1. `OUTCOME_UNKNOWN` must not automatically release the same-name serialization fence The proposal says that an unknown job no longer holds the same-name fence, even though the old executor may still be running and may commit later. This permits the following sequence: 1. job A for index `idx` loses contact and becomes `OUTCOME_UNKNOWN`; 2. its same-name fence is released; 3. the operator resolves or re-issues the operation, and job B establishes a new known state for `idx`; 4. the old executor for job A commits afterwards. The late commit can overwrite, remove, or reintroduce state after job B has finished. Holding a catalog/cluster admission slot does not prevent this when the configured limit is greater than one, and automatic retention expiry makes the same race possible without an operator decision. While executor termination cannot be proved, the table/index-name fence must remain. Retention expiry may archive the job record, but it must not silently release this correctness fence. If 4.2 needs an escape hatch, make it an explicit force-resolution operation requiring ALTER privilege, a non-empty note, and a warning that a late external commit remains possible. Please add a test in which the old executor commits after a force-release. ## 2. Proven no-commit and metadata refresh are independent decisions Section 3 currently maps typed commit conflict, already-exists, and not-found results to `FAILED` with no refresh. A commit conflict proves that this job did not commit, but it also proves that the external dataset advanced. Likewise, already-exists or not-found can reveal that the state changed between pre-dispatch reconciliation and execution. Doris must refresh authoritative metadata in these cases even if the mutation job itself ends as `FAILED`. The IF modifiers also need operation-specific handling in this race window: - `CREATE INDEX IF NOT EXISTS` receiving already-exists should re-read authoritative metadata and finish as a no-op when the documented comparison matches; - `DROP INDEX IF EXISTS` receiving not-found should refresh and finish as a no-op; - a mismatch may still fail, but it must not leave stale metadata behind. Please separate mutation attribution from cache-coherence behavior instead of making refresh conditional on a successful mutation. ## 3. Operator resolution is not yet a durable, implementable state transition The delta says that resolution records a note, refreshes metadata, and releases the slot, but it does not define: - the SQL or administrative API; - the required privilege; - the durable target state and persisted `resolved_by` / `resolved_at` information; - whether acknowledgement and resolution are distinct operations; - CAS behavior against concurrent resolution or DDL; - replay behavior if FE fails between refresh and slot release. Without these rules, the resolution and expiry cases in the acceptance suite cannot be implemented deterministically. Since there is no job-specific evidence, the resulting state should also avoid claiming that the original mutation outcome became known. ## Additional implementation constraints - **BE resource isolation:** catalog-wide and cluster-wide job counts do not prevent multiple one-shot builds from concentrating on one BE. The BE path needs a bounded dedicated executor, per-BE admission, and scheduler-aware selection. A potentially long lance-c call must not occupy a general RPC handler thread. The design should also state how native-memory pressure is guarded so one build cannot take down the BE. - **`file://` enforcement:** one registered FE and one registered BE does not prove that they share a filesystem namespace. Either restrict this to a verifiable co-located test deployment or define the flag explicitly as an operator assertion and document what Doris can and cannot check. - **REST provider profiles:** define how a code-defined profile is selected and versioned. The generic REST profile should remain read-only unless an allowlisted profile supplies the per-operation status, transaction, or synchronous-completion contract. Doris should neither infer a profile from the URI nor accept user-defined status strings. The FE Java-SDK metadata-read direction is feasible with the pinned SDK's index-description APIs. The implementation should still parse only an allowlist of authoritative fields and enforce strict size bounds on index details before exposing `Properties`. Once the three correctness issues above are resolved and the implementation constraints are pinned, the design should be ready for approval. -- 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]
