-
Notifications
You must be signed in to change notification settings - Fork 28
deps: upgrade aws-sdk-go-v2 dependency #3368
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Summary of ChangesHello @wk989898, 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 updating the 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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request upgrades the aws-sdk-go-v2 dependencies to resolve an endpoint issue. The dependency updates in go.mod and go.sum are correct.
I have one suggestion to improve the error handling in a related file, which is now more robust with the upgraded SDK.
Additionally, the pull request description mentions adding a test for the fixed error, but it appears to be missing from the changes. Please consider adding the test case to ensure the fix is verified and prevent future regressions.
| github.com/aws/aws-sdk-go-v2 v1.40.0 | ||
| github.com/aws/aws-sdk-go-v2/config v1.32.2 | ||
| github.com/aws/aws-sdk-go-v2/credentials v1.19.2 | ||
| github.com/aws/aws-sdk-go-v2/service/glue v1.134.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this upgrade of aws-sdk-go-v2, there's an opportunity to improve error handling in pkg/sink/codec/avro/glue_schema_registry.go. Currently, error handling for EntityNotFoundException relies on string matching (e.g., lines 303 and 317). This is fragile and not idiomatic.
The recommended way with aws-sdk-go-v2 is to use type assertion with errors.As. This makes the code more robust against changes in error messages.
For example, you could refactor the error handling in getSchemaByName and getSchemaByID like this:
import (
"errors"
"github.com/aws/aws-sdk-go-v2/service/glue/types"
)
// ... in getSchemaByName and getSchemaByID
var e *types.EntityNotFoundException
if errors.As(err, &e) {
return false, "", nil
}
return false, "", errors.Trace(err)|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 3AceShowHand, tenfyzhong The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[LGTM Timeline notifier]Timeline:
|
|
/retest-required |
|
/retest |
| assert "ErrDispatcherFailed" in resp.text, f"{resp.text}" | ||
|
|
||
| # create changefeed fail because glue schema config is invalid | ||
| url = BASE_URL1_V2+"/changefeeds" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be compatible with next-gen mode, please add the parameter keyspace=keyspace1 to the URL.
|
[FORMAT CHECKER NOTIFICATION] Notice: To remove the 📖 For more info, you can check the "Contribute Code" section in the development guide. |
What problem does this PR solve?
Issue Number: ref pingcap/tiflow#12424
What is changed and how it works?
According to the comment aws/aws-sdk-go-v2#2370 (comment), we upgrade the dependency and add a test for this error.
Check List
Tests
Questions
Will it cause performance regression or break compatibility?
Do you need to update user documentation, design documentation or monitoring documentation?
Release note