tomsanbear commented on PR #610: URL: https://github.com/apache/arrow-rs-object-store/pull/610#issuecomment-4276938715
@pdeva — rebased onto current main and addressed tustvold's three review comments. Branch: [tomsanbear/arrow-rs-object-store:feat/conditional-deletes](https://github.com/tomsanbear/arrow-rs-object-store/tree/feat/conditional-deletes) (three commits on top of your two). The core change swaps `DeleteOptions.if_match: Option<String>` for `condition: Option<UpdateVersion>` to match `PutMode::Update(UpdateVersion)`. I also dropped the http override (the default trait impl handles both branches strictly better for that backend) and switched gcp to `x-goog-if-generation-match` via the existing `VERSION_MATCH` constant. Couple of calls worth flagging since they diverge from your original direction: I dropped the `*` wildcard and multi-etag test cases. Neither maps cleanly onto `UpdateVersion`'s `{ e_tag, version }` shape without reintroducing string parsing into a structured type. If wildcard or multi-etag matters for downstream use I'd rather add them as a separate field (or a small `match_mode` enum) than overload `UpdateVersion` — but for the scope of tustvold's three comments, keeping the type tight was cleaner. I also kept AWS and Azure on the single-object DELETE-with-`If-Match` path you originally wrote rather than routing through the batch endpoints. S3 does support per-entry `<ETag>` in the `DeleteObjects` XML body and Azure's blob batch supports per-sub-request `If-Match`, so the batch path would work — but your single-object path is the cleaner unary contract and lines up with tustvold's framing of `delete_opts` as the unary counterpart to `delete_stream`. No reason to churn it. `delete_opts_race_condition` stays defined but is now only wired for AWS/Azure/GCS, not Memory/Local. The "exactly one winner" invariant requires server-side atomic precondition evaluation; Memory and Local do check-then-delete non-atomically, so the test was passing by luck. Making Memory atomic is a reasonable follow-up but out of scope here. Integration tests now use `UpdateVersion::from(put_result)` so each backend picks the field it needs — GCS reads `.version`, everything else reads `.e_tag`. fmt / test / clippy green locally; integration tests still pending a CI run. Let me know if you want to pull these onto your branch and keep driving, or if you'd rather I open a follow-up PR superseding #610 with you as co-author. Either works. Holding off on the new PR until you respond. -- 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]
