Read Consistency Strategy: Adds hub region header for LastCommittedWriteRegion strategy.#5815
Read Consistency Strategy: Adds hub region header for LastCommittedWriteRegion strategy.#5815
Conversation
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
@sdkReviewAgent-2 |
| readConsistencyStrategy.Value.ToString()); | ||
|
|
||
| requestMessage.Headers.Set( | ||
| HttpConstants.HttpHeaders.ShouldProcessOnlyInHubRegion, |
There was a problem hiding this comment.
🟡 Recommendation — Hub header applies to all operation types (including writes)
The new hub region header is set unconditionally whenever ReadConsistencyStrategy has a value — regardless of the request's OperationType. When ReadConsistencyStrategy is configured at the client level (CosmosClientOptions.ReadConsistencyStrategy), every request flowing through SendAsync gets this header — including writes (CreateItemAsync, ReplaceItemAsync, UpsertItemAsync, DeleteItemAsync, PatchItemAsync).
The sibling method ValidateAndSetConsistencyLevelAsync explicitly validates against OperationType and ResourceType via:
ValidationHelpers.IsValidConsistencyLevelOverwrite(
operationType: requestMessage.OperationType,
resourceType: requestMessage.ResourceType)This method performs no such check. Since ReadConsistencyStrategy is semantically a read concept (the name, docs, and enum description all say "read and query operations"), routing writes to the hub region is likely unnecessary overhead — and in multi-region write scenarios, it could add latency by forcing writes away from the nearest write region.
Concrete scenario: A customer sets CosmosClientOptions { ReadConsistencyStrategy = Session } for consistent reads. Every CreateItemAsync call now sends x-ms-cosmos-hub-region-processing-only: True, potentially routing writes to a distant hub region instead of the nearest one.
Suggestion: Either:
- Add an operation-type guard (skip hub header for write operations), or
- Add a comment/test documenting that writes intentionally get hub routing when
ReadConsistencyStrategyis active, and confirm the backend ignores the header on writes.
Note: This is also a cross-SDK divergence — the Java SDK's getEffectiveReadConsistencyStrategy() explicitly returns DEFAULT for write operations, filtering them out. The .NET approach (proactive hub routing on first request) is already a deliberate divergence from Java's reactive approach (hub routing only after 404/1002 retry), so this may be intentional — but the write-operation case deserves explicit confirmation.
There was a problem hiding this comment.
Hub region header is now only set when ReadConsistencyStrategy == LastCommittedWriteRegion AND the operation is a read (OperationTypeExtensions.IsReadOperation()).
Write operations will never get this header regardless of the strategy configured.
|
✅ Review complete (55:02) Posted 2 inline comment(s). Steps: ✓ context, correctness, cross-sdk, design, history, past-prs, synthesis, test-coverage |
kushagraThapar
left a comment
There was a problem hiding this comment.
Now that we always route requests to hub region, what happens if customers set this in case of multi-writer account?
| HttpConstants.HttpHeaders.ReadConsistencyStrategy, | ||
| readConsistencyStrategy.Value.ToString()); | ||
|
|
||
| requestMessage.Headers.Set( |
There was a problem hiding this comment.
can this get set even for write operations? If so, it might create a side effect.
There was a problem hiding this comment.
Hub region header is now only set when ReadConsistencyStrategy == LastCommittedWriteRegion AND the operation is a read (OperationTypeExtensions.IsReadOperation()).
Write operations will never get this header regardless of the strategy configured.
| HttpConstants.HttpHeaders.ReadConsistencyStrategy, | ||
| readConsistencyStrategy.Value.ToString()); | ||
|
|
||
| requestMessage.Headers.Set( |
There was a problem hiding this comment.
Hubregion processing should only be triggered by a single ReadConsistencyStartegy enum value - like LastCommittedWriteRegion or similar. Not for all ReadConsistencyStartegies for sure!
There was a problem hiding this comment.
Hub region header is now only set when ReadConsistencyStrategy == LastCommittedWriteRegion AND the operation is a read (OperationTypeExtensions.IsReadOperation()).
IsReadOperation includes: Read, ReadFeed, Query, SqlQuery, Head, HeadFeed and MetadataCheckAccess
FabianMeiswinkel
left a comment
There was a problem hiding this comment.
Hubregion processing should only be triggered by a single ReadConsistencyStartegy enum value - like LastCommittedWriteRegion or similar. Not for all ReadConsistencyStartegies for sure!
…stencyStrategy and only for read operation type
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
| using CosmosClient client = MockCosmosUtil.CreateMockCosmosClient( | ||
| accountConsistencyLevel: Cosmos.ConsistencyLevel.Strong, | ||
| customizeClientBuilder: builder => builder.WithReadConsistencyStrategy(Cosmos.ReadConsistencyStrategy.LatestCommitted)); | ||
| customizeClientBuilder: builder => builder.WithReadConsistencyStrategy(Cosmos.ReadConsistencyStrategy.LastCommittedWriteRegion)); |
There was a problem hiding this comment.
🟡 Recommendation — Testing: Existing test replaced rather than extended
Per review rules: "new feature tests should NOT replace existing test coverage for related features."
This test previously validated LatestCommitted at the client level. It now tests LastCommittedWriteRegion instead — meaning the only unit test for LatestCommitted client-level propagation is gone.
While LatestCommitted is still covered at the request level via the DataRow("LatestCommitted") in ReadConsistencyStrategyRequestOptionSetsHeaders, the client-builder → handler fallback path (this.RequestedClientReadConsistencyStrategy) is no longer exercised for it.
Suggestion: Either parameterize this test over multiple strategies or add back a LatestCommitted variant alongside the new LastCommittedWriteRegion one:
[TestMethod]
[DataRow("LatestCommitted")]
[DataRow("LastCommittedWriteRegion")]
public async Task ReadConsistencyStrategyClientLevelApplied(string strategyName) { ... }| HttpConstants.HttpHeaders.ReadConsistencyStrategy, | ||
| readConsistencyStrategy.Value.ToString()); | ||
|
|
||
| if (readConsistencyStrategy.Value == Cosmos.ReadConsistencyStrategy.LastCommittedWriteRegion |
There was a problem hiding this comment.
🟡 Recommendation — Design: Consider interaction with CrossRegionHedgingAvailabilityStrategy
When CrossRegionHedgingAvailabilityStrategy and LastCommittedWriteRegion are both active, the hedging strategy clones the request (including the ShouldProcessOnlyInHubRegion header) and sends copies to multiple regions in parallel. Non-hub regions will return 403/3, which IsFinalResult() does NOT treat as final — so each hedged copy retries internally, generating cascading 403/3 traffic.
Concrete scenario: 3-region account with hedging enabled + LastCommittedWriteRegion read → 3 parallel requests → 2 get 403/3 and retry → generates ~5+ backend requests for one logical read, with wasted RUs and added latency.
Why it matters: The combination is semantically contradictory — hedging spreads across regions, but hub-only routing rejects all non-hub regions. This isn't a correctness bug (it eventually converges), but it's wasteful and could confuse diagnostics.
Suggestion: Either:
- Add a check in
ShouldHedgeto skip hedging whenShouldProcessOnlyInHubRegionis set, or - Document this as an unsupported combination, or
- At minimum, add a code comment noting this interaction for future maintainers.
|
✅ Review complete (07:10) Posted 7 inline comment(s). Steps: ✓ context, correctness, cross-sdk, design, history, past-prs, synthesis, test-coverage |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
kushagraThapar
left a comment
There was a problem hiding this comment.
LGTM, thanks @aavasthy for the changes.
Pull Request Template
Description
This PR introduces a new LastCommittedWriteRegion value to the ReadConsistencyStrategy enum, enabling reads to be routed to the hub (write) region so they always reflect the most recent writes regardless of which region the client is connected to. When this strategy is set either at the client or request level and the operation is a read (point read, query, change feed, or ReadMany), the SDK additionally sets the ShouldProcessOnlyInHubRegion header so the backend routes the request to the write region. The header is only applied for read operations and is not set for writes.
Type of change
Please delete options that are not relevant.
Closing issues
To automatically close an issue: closes #IssueNumber