azure: support managed identity and fix a goroutine hang issue#3094
Conversation
ti-chi-bot
bot
added
release-note
Summary of ChangesHello @wlwilliamx, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on a significant upgrade of the Azure SDK and related authentication libraries to enable robust support for Managed Identity. Beyond the Azure-specific updates, it also includes a comprehensive refresh of various other Go module dependencies, ensuring the project benefits from the latest features, performance improvements, and security patches across its entire dependency tree. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request, titled 'feat(azure): update azure sdk to support managed identity', also includes a large number of dependency updates that go far beyond the stated scope. Most notably, it appears that updating pingcap/tidb and related dependencies has transitively caused major version upgrades for critical libraries like Apache Arrow (from v12 to v18) and the removal of xitongsys/parquet-go.
These are significant, high-risk changes that are not documented in the pull request description. Major dependency upgrades can introduce subtle bugs, performance regressions, or breaking API changes that are hard to catch without dedicated testing.
To manage risk and ensure a focused review, I strongly recommend splitting this pull request:
- A PR that only updates the Azure SDK and its direct dependencies to support managed identity.
- A separate PR for the
pingcap/tidbecosystem update. This PR must include a detailed description explaining the rationale, an impact analysis of the major version changes (especially for Arrow), and a summary of the testing performed to ensure correctness and performance are not affected.
Bundling these unrelated changes makes the review process difficult and significantly increases the risk of introducing unintended issues.
|
/test all |
ti-chi-bot
bot
added
size/XXL
wlwilliamx
changed the title
ti-chi-bot
bot
added
needs-1-more-lgtm
| @@ -69,14 +69,14 @@ func getExternalStorage( | |||
| }) | |||
| if err != nil { | |||
| retErr := errors.ErrFailToCreateExternalStorage.Wrap(errors.Trace(err)) | |||
There was a problem hiding this comment.
just call errors.WrapError(ErrFailToCreateExternalStorage, err) is the correct usage
| if err != nil { | ||
| retErr := errors.ErrFailToCreateExternalStorage.Wrap(errors.Trace(err)) | ||
| return nil, retErr.GenWithStackByArgs("creating ExternalStorage for s3") | ||
| return nil, retErr.GenWithStackByArgs("creating ExternalStorage") |
| if cancelCreate != nil { | ||
| cancelCreate() | ||
| } | ||
| err = errors.ErrExternalStorageAPI.Wrap(err).GenWithStackByArgs("Create") |
| cancel() | ||
| } | ||
| if err != nil { | ||
| err = errors.ErrExternalStorageAPI.Wrap(err).GenWithStackByArgs("Close") |
| createCtx context.Context | ||
| } | ||
|
|
||
| func (w *blockingCreateCtxWriter) Write(_ context.Context, _ []byte) (int, error) { |
There was a problem hiding this comment.
why not also select on the passed in context ?
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 3AceShowHand, lidezhu The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[LGTM Timeline notifier]Timeline:
|
wlwilliamx
added
the
needs-cherry-pick-release-8.5
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
|
In response to a cherrypick label: new pull request created to branch |
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
4 participants
What problem does this PR solve?
Issue Number: close #3093
TiCDC writes directly to Azure Blob Storage for the azblob sink. This PR adds Azure Managed Identity / Workload Identity token auth support and fixes a potential goroutine hang in cloud storage uploads.
What is changed and how it works?
Check List
Tests
Manual test steps:
Questions
Will it cause performance regression or break compatibility?
No expected regression; existing SAS-based configs remain supported.
Do you need to update user documentation, design documentation or monitoring documentation?
N/A
Release note