Skip to content

Conversation

@mdaigle
Copy link
Contributor

@mdaigle mdaigle commented Oct 20, 2025

Description

Backports #3625

@mdaigle mdaigle added this to the 6.1.3 milestone Oct 20, 2025
@mdaigle mdaigle marked this pull request as ready for review October 20, 2025 22:12
@mdaigle mdaigle requested a review from a team as a code owner October 20, 2025 22:12
Copilot AI review requested due to automatic review settings October 20, 2025 22:12
Copy link
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 backports a BAG (Basic Availability Group) failover fix from #3625 to the 6.1 branch. The primary change introduces an AppContext switch IgnoreServerProvidedFailoverPartner that allows applications to ignore failover partner information provided by SQL Server, enabling explicit control over failover behavior (e.g., when using custom ports).

Key changes:

  • Added new IgnoreServerProvidedFailoverPartner AppContext switch with default value of false
  • Renamed ServerProvidedFailOverPartner to ServerProvidedFailoverPartner for consistent casing throughout the codebase
  • Modified failover logic to respect the new switch when updating pool group failover partner designations

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/LocalAppContextSwitches.cs Added new AppContext switch property and documentation, fixed typo "regardsless" → "regardless"
src/Microsoft.Data.SqlClient/tests/Common/LocalAppContextSwitchesHelper.cs Extended test helper to support the new IgnoreServerProvidedFailoverPartner switch
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs Refactored failover partner property, implemented switch-aware failover logic, updated all references to use consistent casing
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs Applied same failover partner changes as netfx version for cross-platform consistency
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlCommand.cs Updated property reference to use corrected casing
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlCommand.cs Updated property reference to use corrected casing

@codecov
Copy link

codecov bot commented Oct 20, 2025

Codecov Report

❌ Patch coverage is 44.44444% with 25 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.16%. Comparing base (b1bf339) to head (2a01f50).
⚠️ Report is 2 commits behind head on release/6.1.

Files with missing lines Patch % Lines
...crosoft/Data/SqlClient/SqlInternalConnectionTds.cs 36.84% 12 Missing ⚠️
...crosoft/Data/SqlClient/SqlInternalConnectionTds.cs 36.84% 12 Missing ⚠️
...icrosoft/Data/SqlClient/LocalAppContextSwitches.cs 80.00% 1 Missing ⚠️
Additional details and impacted files
@@               Coverage Diff               @@
##           release/6.1    #3702      +/-   ##
===============================================
+ Coverage        67.50%   70.16%   +2.66%     
===============================================
  Files              279      279              
  Lines            61750    61764      +14     
===============================================
+ Hits             41685    43338    +1653     
+ Misses           20065    18426    -1639     
Flag Coverage Δ
addons 90.82% <ø> (ø)
netcore 73.09% <48.00%> (+3.60%) ⬆️
netfx 70.03% <48.00%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@benrr101 benrr101 left a comment

Choose a reason for hiding this comment

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

Some discrepancies between this and the other ports...

@paulmedynski paulmedynski self-assigned this Oct 24, 2025
Copy link
Contributor

@paulmedynski paulmedynski left a comment

Choose a reason for hiding this comment

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

Where are the new tests?

* Add IgnoreServerProvidedFailoverPartner app context switch.

* Add behavior skip to netfx.

* Consolidate to single property for failover partner value.

* Rework checks to preserve server provided value, but ignore it.

* Fix import.

* Skip server failover partner override in �LoginNoFailover method. Add simulated server test coverage.
@mdaigle mdaigle force-pushed the dev/mdaigle/6.1-bag-failover-fix branch from c2fb0e0 to 1573736 Compare October 29, 2025 19:12
@mdaigle
Copy link
Contributor Author

mdaigle commented Oct 29, 2025

I reapplied the cherry-pick and force pushed. This diff should hopefully be cleaner.

benrr101
benrr101 previously approved these changes Oct 29, 2025
Copy link
Contributor

@benrr101 benrr101 left a comment

Choose a reason for hiding this comment

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

Happy to approve if that one comment is expected behavior

apoorvdeshmukh
apoorvdeshmukh previously approved these changes Oct 30, 2025
paulmedynski
paulmedynski previously approved these changes Oct 30, 2025
@mdaigle
Copy link
Contributor Author

mdaigle commented Oct 30, 2025

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@mdaigle mdaigle merged commit 5200419 into release/6.1 Oct 31, 2025
252 checks passed
@mdaigle mdaigle deleted the dev/mdaigle/6.1-bag-failover-fix branch October 31, 2025 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants