Add ChangeFeed AllVersionsAndDeletes mode#4706
Conversation
c2445af to
93a13de
Compare
Two nits from the deep PR review of #4706: - cosmos_operation.rs: the ChangeFeedStartFrom::Now doc claimed AllVersionsAndDeletes *must* resolve Now to a concrete start before persisting, which contradicts the shipped, deliberately-documented per-range Now limitation in query_change_feed. Reworded to match. - change_feed.rs: all_versions_and_deletes_rejects_beginning only asserted is_err(). It now asserts the client-side rejection carries a 400 BadRequest, so a regression to a different status (or one that lets the request reach the emulator) fails the test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
8a6b740 to
7ad4819
Compare
The change feed page iterator now deserializes each item into the caller's
type directly instead of tolerantly unwrapping the wire-format `current`
field. Callers bind `T = ChangeFeedItem<YourDoc>` on `query_change_feed`
and read the post-change document via `current()`, so the whole wire
envelope is preserved and passed up to the customer rather than stripped by
the SDK.
`ChangeFeedItem<T>` models the full envelope: `current`, `previous`,
and `metadata` (with `ChangeFeedMetadata` / `ChangeFeedOperationType`).
All fields beyond `current` are optional, so `LatestVersion` items
(`{ current }` only) deserialize unchanged while the type is also ready for
full-fidelity reads that populate `previous` / `metadata`.
- Add `models::ChangeFeedItem<T>`, `ChangeFeedMetadata`,
`ChangeFeedOperationType` with unit tests covering create/replace/delete
envelopes and the metadata-absent LatestVersion shape.
- Make `ChangeFeedPageIterator` parse-only (drop the unwrap helpers).
- Update `query_change_feed` and iterator rustdoc/examples to the envelope contract.
- Migrate the emulator and split change-feed integration tests to `ChangeFeedItem<MockItem>`.
- CHANGELOG: note the envelope contract and the new types.
This is a breaking change to the unreleased change feed API from #4621.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The real Cosmos emulator's LatestVersion change feed returns a metadata object carrying positional fields (lsn/crts) but no operationType. A required operation_type made ChangeFeedItem<T> fail to deserialize those responses (missing field \operationType\), which surfaced as six failing emulator_tests::cosmos_change_feed::* tests on the windows_stable CI job. Make operation_type Option<ChangeFeedOperationType> with #[serde(default)] so every envelope field is optional and a single type tolerantly serves both the LatestVersion and full-fidelity wire shapes. Accessors now return Option; add a regression unit test for the partial-metadata shape. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add an iterator-level test proving the parse-only iterator hands the whole envelope to the caller, including a partial \metadata\ object (lsn/crts, no operationType) that mirrors the real LatestVersion wire shape. Previously only the envelope type itself was tested with metadata. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
7ad4819 to
d9e57bc
Compare
Stacks on the change-feed wire-envelope work (#4723) and reuses its shared `ChangeFeedItem<T>` type. Adds only the full-fidelity delta: - driver: `CosmosOperation::change_feed_all_versions_and_deletes` factory and a `full_fidelity_feed` header flag emitting `A-IM: Full-Fidelity Feed` in place of `Incremental Feed`. - driver in-memory emulator: parse the `a-im` header and synthesize a `create` envelope for full-fidelity reads. - SDK: `ChangeFeedMode::AllVersionsAndDeletes` and mode dispatch in `query_change_feed` selecting the factory; reject `ChangeFeedStartFrom::Beginning` (400 BadRequest) since intermediate versions and deletes only exist within the retention window. - Tests: iterator create/replace/delete envelope decoding plus emulator end-to-end coverage (latest-version, full-fidelity, Beginning rejection). CHANGELOG entries for both crates. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
d9e57bc to
2032bc1
Compare
The vnext Cosmos emulator does not honor the change feed wire-format
header and returns bare documents instead of the `{ "current": ... }`
envelope. The parse-only iterator deserialized those directly into
`ChangeFeedItem<T>`, leaving `current` empty and panicking the emulator
change feed tests.
Give `ChangeFeedItem<T>` a tolerant `Deserialize`: an object carrying any
reserved envelope key (`current`/`previous`/`metadata`) is parsed as the
envelope; otherwise the whole document is mapped onto `current` so no
data is lost on non-enveloping backends. Real Cosmos still envelopes, so
`previous`/`metadata` are preserved there.
Add unit tests for the flat shape and a delete envelope (which carries no
`current`), and note the behavior in the changelog.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Add the `ChangeFeedPolicy` model type and `ContainerProperties::with_change_feed_policy()` so containers can configure a full-fidelity retention window (`retentionDuration`, in minutes), which the service requires to serve `ChangeFeedMode::AllVersionsAndDeletes` reads. Add integration coverage for AVAD: - Emulator tests (emulator-only; the vnext Linux emulator does not support full fidelity): a create/replace/delete of one document surfaces as three envelopes with chained `previousImageLsn`, and creates fan out once each across multiple physical partitions. - A live split test that captures a `Now` token, forces a real partition split, then verifies post-split creates and deletes resume exactly once with no baseline replay. - Unit tests for `ChangeFeedPolicy` serialization and round-tripping. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@copilot resolve the merge conflicts in this pull request |
Co-authored-by: yumnahussain <161499581+yumnahussain@users.noreply.github.com>
Co-authored-by: yumnahussain <161499581+yumnahussain@users.noreply.github.com>
Resolved. I merged |
|
@copilot resolve the merge conflicts in this pull request |
…nvelope' into yumnahussain/yumnahussain-cosmos-changefeed-avad # Conflicts: # sdk/cosmos/azure_data_cosmos/tests/in_memory_emulator_tests/mod.rs # sdk/cosmos/azure_data_cosmos_driver/src/in_memory_emulator/operations.rs Co-authored-by: yumnahussain <161499581+yumnahussain@users.noreply.github.com>
Resolved the new merge conflicts and merged the updated stacked base branch into this PR in commit |
|
Azure Pipelines: Successfully started running 1 pipeline(s). 2 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Route full-fidelity (AllVersionsAndDeletes) change feed away from Gateway 2.0, which does not forward the A-IM header and is excluded by the GW2 spec; without this an AVAD read against a GW2-enabled account silently downgrades to LatestVersion. Thread the full_fidelity_feed flag into is_operation_supported_by_gateway_v2() at both routing call sites, with eligibility + routing unit tests. Also format the column-0 code left by the auto-fix commits so fmt --check passes, restore the unrelated CHANGELOG entries that had been dropped, correct the #4706 CHANGELOG and doc wording, and make the live split test drain deadline/predicate-based so lagging post-split deletes are not missed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
All versions and deletes mode can only start from "now" or resume from a continuation token within the container's retention / continuous-backup window. Per the Cosmos DB docs and the .NET/Java SDKs, reading from an arbitrary point in time is not supported in this mode, yet the client allowed PointInTime and the Beginning-rejection message even recommended it. Extend the client-side guard to reject ChangeFeedStartFrom::PointInTime (alongside Beginning) for AllVersionsAndDeletes, correct the error and CHANGELOG wording, and add an emulator test asserting the rejection. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Reworks the AllVersionsAndDeletes change feed to defer unsupported start-position rejection to the service and to make continuation tokens mode-aware, per review feedback on PR #4706. - Remove the client-side guard that rejected `Beginning`/`PointInTime` for AllVersionsAndDeletes; the service gates these. The in-memory emulator now stands in for the service: it parses `If-Modified-Since` and returns `400 Bad Request` for a full-fidelity read that starts from the beginning (no `If-None-Match`) or a point in time (`If-Modified-Since`). `Now` and continuation-token resume are allowed. - Encode the feed mode in the continuation token (`cfm`), and reject a resume that switches modes with a clear `400`. Only the full-fidelity marker is stored, so LatestVersion and query tokens are unchanged and backward-compatible. - Move the two AVAD start-rejection assertions onto the streamed first page (the request is now issued lazily) and add an in-memory `PointInTime` rejection test alongside the existing `Beginning` one. - Apply reviewer doc suggestions and scrub "full fidelity" from the public docs in favor of "all versions and deletes"; drop the verbose Gateway 2.0 eligibility doc comment. - Update both CHANGELOGs to describe service gating and mode-in-token. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (3)
sdk/cosmos/azure_data_cosmos/src/models/container_properties.rs:470
- This constructor can silently disable the mode it promises to enable:
Duration::from_secs(59)becomesretentionDuration: 0, while 90 seconds is shortened to 60 seconds. Since the wire format only supports whole minutes, make this boundary fallible and reject zero or non-whole-minute durations instead of changing the caller's requested policy.
pub fn all_versions_and_deletes(retention: Duration) -> Self {
Self {
retention_duration_minutes: (retention.as_secs() / 60).min(i32::MAX as u64) as i32,
sdk/cosmos/azure_data_cosmos/src/clients/container_client.rs:897
- This contradicts the PR description, which tells users to use an in-retention
PointInTimefor this mode. The implementation and new emulator tests reject that start instead. Please resolve which contract is intended and align the code, tests, changelog, API docs, and PR description so reviewers and users are not given opposite behavior.
/// * Only [`ChangeFeedStartFrom::Now`] or resuming from a continuation token
/// is supported. [`ChangeFeedStartFrom::Beginning`] and
/// [`ChangeFeedStartFrom::PointInTime`] are **not** supported, because
/// intermediate versions and deletes are only retained within the
/// container's retention / continuous-backup window. The service gates
/// this and rejects such a request with `400 Bad Request`.
sdk/cosmos/azure_data_cosmos/tests/split_tests/cosmos_change_feed_split.rs:435
- These lines are not rustfmt-formatted, so the repository's mandatory formatting check will fail. Run
cargo fmt -p azure_data_cosmosover the complete change; the same formatter will also wrap the other newly added long expressions in this test.
got_creates.sort();
expected_creates.sort();
LatestVersion reads can still carry partial change metadata (lsn/crts without an operation type), which ChangeFeedItem accepts. The doc for query_change_feed claimed only current() is populated in that mode, which would lead callers to ignore metadata the service can return. Reword to note metadata() may also be present while previous() is not. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (3)
sdk/cosmos/azure_data_cosmos/tests/emulator_tests/cosmos_change_feed.rs:895
- A single
next()only polls one physical range:UnorderedMerge::next_pageadvances one child per page (unordered_merge.rs:24-26,78-104). Since this test deliberately creates 2+ ranges, writes routed to unpolled ranges establish theirNowposition only after the writes and can be omitted, making the fan-out test time out/fail. Poll once per current feed range before writing.
// Prime the per-range `Now` positions before writing.
let primed = iterator
.next()
.await
.expect("change feed stream always yields a page")?;
sdk/cosmos/azure_data_cosmos/src/models/container_properties.rs:446
- The stated 1-hour minimum is inaccurate: the official Azure Cosmos DB .NET
ChangeFeedPolicydocumentation configures a 5-minute full-fidelity retention window, and the wire setting has minute granularity. This public documentation would incorrectly discourage supported shorter windows; avoid hard-coding an unverified range since validation is intentionally delegated to the service.
/// The retention window has minute granularity. On the service it must fall
/// within the supported range (currently 1 hour to 30 days) when this mode
/// is enabled; this type does not enforce that range, leaving the service as the
/// source of truth.
sdk/cosmos/azure_data_cosmos/tests/split_tests/cosmos_change_feed_split.rs:435
- These added statements are not rustfmt-indented, so the repository's mandatory
cargo fmt --checkwill fail. Runcargo fmt -p azure_data_cosmosbefore merging.
got_creates.sort();
expected_creates.sort();
NaluTripician
left a comment
There was a problem hiding this comment.
Local deep review of the AllVersionsAndDeletes change feed mode. Overall this is a high-quality, unusually well-tested PR — the two dangerous wire paths (Gateway 2.0 A-IM downgrade protection, and continuation-token mode binding) are both handled correctly and tested. No blocking wire/correctness bugs.
The inline notes below are, in priority order: one High (lossy Now resume undercuts AVAD's core delete guarantee), three Medium (client-side fail-fast, sub-minute retention truncation, preview-gating), two Low, and a fmt nit. The top ask before "done" is the lossy Now resume.
Make AllVersionsAndDeletes feeds lossless across a resume and tighten the retention policy surface, per nalutripician's review of #4706. - Driver: pin every range to a concrete start ETag on the first drain of a fresh full-fidelity feed (prime-on-first-drain in UnorderedMerge, gated by the planner on `full_fidelity_feed && !is_resume`). A range that is never served before a checkpoint now still resumes from its true start instead of a resume-time `Now`, so no versions/deletes in the gap are dropped. LatestVersion is unchanged. Adds driver priming unit tests plus two integration tests covering the AVAD pin and the LatestVersion no-prime path. - ChangeFeedPolicy: round sub-minute retention up to a 1-minute floor instead of truncating to 0 (which silently disabled the mode); make `retention_duration_minutes` private with a getter and clamp negatives on serialize so an invalid value can never reach the wire. - change_feed_iterator: assert `crts` (conflict resolution timestamp) in the AVAD envelope deserialization test. - Docs: update the container_client `Now` caveat to reflect lossless resume; minor split-test re-indentation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
sdk/cosmos/azure_data_cosmos/src/models/container_properties.rs:489
Duration::as_secs()truncates sub-second values, so a nonzero retention such asDuration::from_nanos(1)serializes as0and disables AllVersionsAndDeletes, contrary to this method's stated round-up contract. Includesubsec_nanos()when deciding whether a partial minute remains.
retention_duration_minutes: retention.as_secs().div_ceil(60).min(i32::MAX as u64)
as i32,
| /// * [`ChangeFeedMode::LatestVersion`] (default) — the latest version of | ||
| /// each created or replaced item. `current()` holds the item and | ||
| /// `metadata()` may also be present (for example `lsn` and the commit | ||
| /// timestamp), while `previous()` is not populated. |
| /// * When starting from [`ChangeFeedStartFrom::Now`], every range is pinned | ||
| /// to its concrete starting position before the first page is returned, so | ||
| /// a range that is never served before a checkpoint still resumes from its | ||
| /// true start rather than resume-time. No intermediate versions or deletes | ||
| /// are dropped across a resume. `Now` is deliberately **not** rewritten to | ||
| /// a concrete [`ChangeFeedStartFrom::PointInTime`], which would change its | ||
| /// semantics; each range instead captures its own server continuation. |
NaluTripician
left a comment
There was a problem hiding this comment.
Re-review of the fixup commit ("Address AVAD change-feed review comments"). Approving with nits.
I traced the H1 priming fix end-to-end against the source (plan_operation -> build_unordered_merge -> prime_children -> snapshot_state -> planner is_resume) and it genuinely closes the lossy-Now resume for the documented usage pattern (pull >=1 page, then checkpoint). The new integration test proves the never-served range is pinned and re-sends its ETag on resume, and latest_version_does_not_prime_ranges locks the mode gate. The other fixes all check out too: div_ceil round-up (overflow-safe, 0->0), the private retention field + serialize_with clamp on every read path (no direct field reads left in the crate), and the added crts assertion.
One residual Medium is worth a fast-follow (inline on planner.rs): a checkpoint taken before the first page still resumes from a stale Now. It's a narrow-but-legal, silent, untested path. Plus two minor nits. None are blocking, hence approve-with-nits — but I'd genuinely like to see the pre-first-page case closed before this is leaned on as fully stable (it's the last sliver of the exact guarantee this PR set out to provide).
| // drop the versions/deletes in the gap. On resume every range already | ||
| // carries a saved continuation (the prior session primed them), so priming | ||
| // is only needed on a fresh start. | ||
| let prime_on_first_drain = operation.request_headers().full_fidelity_feed && !is_resume; |
There was a problem hiding this comment.
[Medium — residual H1 gap] A checkpoint taken before the first page still resumes from a stale Now and silently drops the gap.
Priming is gated full_fidelity_feed && !is_resume, and upstream is_resume = saved_tokens.is_some() (planner.rs:258). I traced the pre-first-page path:
query_change_feed().awaiteagerly builds theUnorderedMergewith all N children (cosmos_driver.rs:2704), andto_continuation_token()is callable before the first poll (planisSomeat construction — it only errors mid-fetch).- A snapshot taken before
.next()skips every never-polled child becauseserver_continuationisNone(unordered_merge.rs:274), yieldingactive_tokens: []+start_from: Now. - On resume,
Some([]).is_some() == true→prime_on_first_drain = false→ priming is disabled → every range falls back to the persistedNowmarker and reads from resume-timeNow, dropping exactly the versions/deletes in the gap — the same failure H1 fixes, relocated to the pre-first-page window.
This is legal API usage (checkpoint an iterator before pulling a page) and is currently untested.
Suggested fix: treat an empty-token resume as still-needs-priming, e.g.
let prime_on_first_drain = operation.request_headers().full_fidelity_feed
&& saved_tokens.as_ref().map_or(true, |t| t.is_empty());A fully-drained resume arrives as PipelineNodeState::Drained (handled earlier), not an empty UnorderedMerge, so this stays safe. Please also add a test that snapshots a fresh AVAD pipeline before the first page and asserts resume still pins every range.
| // ETag, which the child has now recorded. Discarding the | ||
| // (item-less) page loses nothing; advance to the next | ||
| // child. | ||
| idx += 1; |
There was a problem hiding this comment.
[Low — defensive] The priming Page arm advances unconditionally without confirming the primed page was item-less.
This is correct under the AVAD invariant that a fresh feed's first start-from-Now poll is a 304 carrying only an ETag. But if a range's first poll ever returns a 304 with no ETag, the child stays Initial, priming still treats it as success (idx += 1), and that range contributes None to the snapshot — so it silently resumes from Now (the H1 gap, for that one range). Unlikely against the real service, but silent rather than an error, and the mock always supplies an ETag so it's untested. Consider a debug_assert! that the primed page is empty, or explicitly handling the no-ETag case.
| while idx < self.children.len() { | ||
| let mut split_retries = 0; | ||
| loop { | ||
| match self.children[idx].next_page(context).await? { |
There was a problem hiding this comment.
[Nit — resiliency + latency] Priming is sequential and fail-all-or-nothing.
This ? propagates a hard error on any single range and aborts the entire first page, even for ranges that would have served data — priming front-loads and serializes every range's first poll, so it strictly widens the window where one bad/slow partition stalls feed startup. It also adds N sequential round-trips (and N RUs) before the first page on a wide fan-out AVAD feed. Functionally fine, but worth (a) confirming the request layer retries transient errors beneath this point, (b) considering priming ranges concurrently, and (c) documenting the extra first-page latency/RU cost, which the comments don't mention.
|
/azp run rust - cosmos - weekly |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Adds the AllVersionsAndDeletes change feed mode.
What it adds
CosmosOperation::change_feed_all_versions_and_deletesfactory and anall_versions_and_deletesheader flag that emitA-IM: Full-Fidelity Feed(instead ofIncremental Feed). Everything else on the wire is identical to LatestVersion. This mode always routes through Gateway 1.0 so the header is never dropped by a Gateway 2.0 downgrade.ChangeFeedMode::AllVersionsAndDeletes.query_change_feedstays the single entry point and dispatches on the mode to pick the factory. In this mode the service returns every intermediate version plus deletes, so theChangeFeedItem<T>envelope'sprevious()andmetadata()(operation type, LSN, commit timestamp, previous-image LSN, TTL-expiry flag) are populated.BeginningandPointInTimeare not supported for this mode; intermediate versions and deletes only exist within the container's retention window. The request is issued and the service returns400 BadRequest. UseNowor resume from a continuation token.400. Only the marker is stored, so LatestVersion and query tokens keep their exact existing payload and legacy tokens are treated as LatestVersion.Known limitations
Nowis re-evaluated per range on first poll and is not converted to a concretePointInTime; a range never polled before a checkpoint can miss changes between the original start and resume. Lossless per-rangeNowis a follow-up.Tests
Iterator create/replace/delete envelope decoding; continuation-token mode encode/accept/reject unit tests; in-memory emulator end-to-end coverage (latest-version, envelope decoding,
Beginning/PointInTimerejection); live emulator create/replace/delete, split/fan-out, andPointInTimerejection tests.