Ppaul/failover ledger endpoint #51947
Conversation
|
TODO:
|
Done |
PallabPaul
requested review from
christothes,
amruthashree18,
andpiccione and
ivarprudnikov
as code owners
There was a problem hiding this comment.
Pull Request Overview
This pull request adds failover endpoint support to the Azure Confidential Ledger SDK, enabling automatic routing to backup ledger instances when the primary endpoint fails. The implementation provides a new failover service that can discover and attempt operations against backup ledger endpoints.
Key changes include:
- Introduction of a new
ConfidentialLedgerFailoverServiceclass that handles failover endpoint discovery and execution - Integration of the failover service into the main
ConfidentialLedgerClient - Version bump from 1.4.1-beta.3 to 1.5.0-beta.1
Reviewed Changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| ConfidentialLedgerFailoverService.cs | New service class implementing failover endpoint discovery and operation execution logic |
| ConfidentialLedgerClient.cs | Integration of the failover service into the main client |
| Azure.Security.ConfidentialLedger.csproj | Version increment to reflect new failover functionality |
| CHANGELOG.md | Documentation of the new failover routing feature |
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
...onfidentialledger/Azure.Security.ConfidentialLedger/src/ConfidentialLedgerFailoverService.cs
Outdated
Show resolved
Hide resolved
...onfidentialledger/Azure.Security.ConfidentialLedger/src/ConfidentialLedgerFailoverService.cs
Outdated
Show resolved
Hide resolved
...onfidentialledger/Azure.Security.ConfidentialLedger/src/ConfidentialLedgerFailoverService.cs
Outdated
Show resolved
Hide resolved
...onfidentialledger/Azure.Security.ConfidentialLedger/src/ConfidentialLedgerFailoverService.cs
Show resolved
Hide resolved
...onfidentialledger/Azure.Security.ConfidentialLedger/src/ConfidentialLedgerFailoverService.cs
Outdated
Show resolved
Hide resolved
...onfidentialledger/Azure.Security.ConfidentialLedger/src/ConfidentialLedgerFailoverService.cs
Show resolved
Hide resolved
| { | ||
| List<Uri> endpoints = await GetFailoverEndpointsAsync(primaryEndpoint, cancellationToken).ConfigureAwait(false); | ||
| Exception last = null; | ||
| foreach (var ep in endpoints) |
There was a problem hiding this comment.
just a minor suggestion, probably not worth doing for this PR since this customer only has one failover: switch to Task.WhenAny or similar for the async failovers method. That should also simplify handling of the requestfailed exceptions.
| var jsonWriterOptions = new System.Text.Json.JsonWriterOptions | ||
| { | ||
| Indented = true, | ||
| Encoder = System.Text.Encodings.Web.JavaScriptEncoder.UnsafeRelaxedJsonEscaping |
There was a problem hiding this comment.
Just curious - what breaks if we use the regular utf8 encoder?
| } | ||
| ms.Position = 0; | ||
| // Wrap in a synthetic Response that mimics the original status/headers but with new content. | ||
| return new SyntheticResponse(currentResponse, ms.ToArray()); |
There was a problem hiding this comment.
The Azure Response ContentStream is settable. I'm wondering if we can just set the currentResponse.contentStream with your memorystream to avoid wrapping and creating the SyntheticResponse.
|
|
||
| if (!string.IsNullOrEmpty(failoverLedgerId)) | ||
| { | ||
| Uri endpoint = new UriBuilder(primaryEndpoint) { Host = $"{failoverLedgerId}.{LedgerDomainSuffix}" }.Uri; |
There was a problem hiding this comment.
Is passing the primaryEndpoint needed here?
| { | ||
| // Include 404 and specific UnknownLedgerEntry error code. | ||
| return ex.Status == 404 || | ||
| string.Equals(ex.ErrorCode, "UnknownLedgerEntry", StringComparison.OrdinalIgnoreCase) || |
There was a problem hiding this comment.
Should UnknownLedgerEntry be retriable? That's a guarantee that no key was written in that transaction right?
| try | ||
| { | ||
| using HttpMessage primaryMessage = CreateGetLedgerEntryRequest(_ledgerEndpoint, transactionId, collectionId, context); | ||
| return await _pipeline.ProcessMessageAsync(primaryMessage, context).ConfigureAwait(false); |
There was a problem hiding this comment.
Forgetting a little: does the pipeline account for Not Ready state? I only remember that the python SDK does not.
Contributing to the Azure SDK
Please see our CONTRIBUTING.md if you are not familiar with contributing to this repository or have questions.
For specific information about pull request etiquette and best practices, see this section.
Changes:
GetLedgerEntry,GetLedgerEntryAsync,GetCurrentLedgerEntry, andGetCurrentLedgerEntryAsyncmethods.