Skip to content

[Internal] ThinProxy Integration: Adds New HttpTimeoutPolicy and Fix Cross Regional Retry Logic#5276

Merged
kirankumarkolli merged 9 commits intomasterfrom
users/kundadebdatta/5275_add_httptimeoutpolicy_for_thin_client
Jul 15, 2025
Merged

[Internal] ThinProxy Integration: Adds New HttpTimeoutPolicy and Fix Cross Regional Retry Logic#5276
kirankumarkolli merged 9 commits intomasterfrom
users/kundadebdatta/5275_add_httptimeoutpolicy_for_thin_client

Conversation

@kundadebdatta
Copy link
Copy Markdown
Member

Pull Request Template

Description

This PR adds the support for the below:

  • Adds new HttpTimeoutPolicy: Primarily create a new http timeout policy to retry the proxy requests faster than the regular Gateway requests. We need an aggressive timeout policy to meet the SLA bar.

  • Adds/ Fixes Cross Regional Retry Logic: Currently the cross regional retry logic for thin client has a gap and it does not resolve the thin proxy endpoints correctly. This PR is fixing that behavior.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Closing issues

To automatically close an issue: closes #5275

@kundadebdatta kundadebdatta self-assigned this Jul 9, 2025
@kundadebdatta kundadebdatta added auto-merge Enables automation to merge PRs thin-client-integration labels Jul 9, 2025
Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good!

@kundadebdatta kundadebdatta changed the title [Internal] Thin Client Integration: Add new HttpTimeoutPolicy and Fix Cross Regional Retry Logic [Internal] Thin Client Integration: Adds New HttpTimeoutPolicy and Fix Cross Regional Retry Logic Jul 9, 2025
@kundadebdatta kundadebdatta requested a review from Copilot July 9, 2025 17:17
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces an aggressive HTTP timeout policy for thin clients and corrects cross-regional retry routing by resolving thin proxy endpoints.

  • Adds HttpTimeoutPolicyForThinClient and plugs it into HttpTimeoutPolicy.GetTimeoutPolicy
  • Extends routing (LocationCache, ClientRetryPolicy, IGlobalEndpointManager, GlobalEndpointManager) to use thin client endpoints
  • Adds a new integration test covering read-item retries across regions in thin client mode

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
tests/.../RegionFailoverTests.cs New test for read‐item retry logic under thin client mode
tests/.../MockSetupsHelper.cs Extends account mock setup to include thin client locations
src/ThinClientStoreClient.cs Enables thin‐client flag in HTTP invocations
src/Routing/LocationCache.cs Adds thin‐client endpoint getters and static IsMetaData helper
src/Routing/IGlobalEndpointManager.cs Exposes thin‐client endpoint collections on the interface
src/Routing/GlobalEndpointManager.cs Implements thin‐client endpoint properties
src/HttpClient/HttpTimeoutPolicyForThinClient.cs New timeout policy class for thin client
src/HttpClient/HttpTimeoutPolicy.cs Updates policy selection logic for thin client
src/ClientRetryPolicy.cs Routes requests to thin‐client endpoints when enabled
Comments suppressed due to low confidence (3)

Microsoft.Azure.Cosmos/src/HttpClient/HttpTimeoutPolicyForThinClient.cs:42

  • [nitpick] The comment seems contradictory: retrieving metadata is idempotent, so it should indicate 'is idempotent' rather than 'is not idempotent'. Consider correcting the comment to reflect the correct idempotency assumptions.
        // The hot path should always be safe to retires since it should be retrieving meta data 

Microsoft.Azure.Cosmos/src/HttpClient/HttpTimeoutPolicyForThinClient.cs:12

  • [nitpick] Public field names should follow PascalCase naming conventions (e.g. 'ShouldRetry') or be made private with public properties to align with .NET naming guidelines.
        public bool shouldRetry;

Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.Tests/PartitionKeyRangeFailoverTests/RegionFailoverTests.cs:214

  • [nitpick] Consider adding a similar test for write scenarios to ensure the HttpTimeoutPolicyForThinClient write path behaves as expected (no retries and correct 503 behavior).
        [TestMethod]

@kirankumarkolli kirankumarkolli changed the title [Internal] Thin Client Integration: Adds New HttpTimeoutPolicy and Fix Cross Regional Retry Logic [Internal] ThinProxy Integration: Adds New HttpTimeoutPolicy and Fix Cross Regional Retry Logic Jul 9, 2025
Copy link
Copy Markdown
Member

@FabianMeiswinkel FabianMeiswinkel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kundadebdatta kundadebdatta changed the title [Internal] ThinProxy Integration: Adds New HttpTimeoutPolicy and Fix Cross Regional Retry Logic [Internal] ThinProxy Integration: Adds New HttpTimeoutPolicy and Fix Cross Regional Retry Logic Jul 10, 2025
jeet1995
jeet1995 previously approved these changes Jul 11, 2025
Comment thread Microsoft.Azure.Cosmos/src/ClientRetryPolicy.cs Outdated
Comment thread Microsoft.Azure.Cosmos/src/ClientRetryPolicy.cs
Copy link
Copy Markdown
Member

@FabianMeiswinkel FabianMeiswinkel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - Thanks

@kundadebdatta kundadebdatta added auto-merge Enables automation to merge PRs and removed auto-merge Enables automation to merge PRs labels Jul 15, 2025
@kundadebdatta kundadebdatta changed the title [Internal] ThinProxy Integration: Adds New HttpTimeoutPolicy and Fix Cross Regional Retry Logic [Internal] ThinProxy Integration: Adds New HttpTimeoutPolicy and Fix Cross Regional Retry Logic Jul 15, 2025
@kirankumarkolli kirankumarkolli merged commit 7acc58a into master Jul 15, 2025
28 checks passed
@kirankumarkolli kirankumarkolli deleted the users/kundadebdatta/5275_add_httptimeoutpolicy_for_thin_client branch July 15, 2025 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-merge Enables automation to merge PRs thin-client-integration

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Thin Client Integration] Add Support For Cross Regional Retry Logic and Define a new HttpTimeoutPolicy for Aggressive Retry for Proxy

5 participants