-
Notifications
You must be signed in to change notification settings - Fork 689
[history server][collector] Migrate to new AWS S3 SDK version #4369
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
base: master
Are you sure you want to change the base?
[history server][collector] Migrate to new AWS S3 SDK version #4369
Conversation
Signed-off-by: 400Ping <fourhundredping@gmail.com>
Signed-off-by: Ping <fourhundredping@gmail.com>
Signed-off-by: 400Ping <fourhundredping@gmail.com>
Signed-off-by: 400Ping <fourhundredping@gmail.com>
8cf62f2 to
b80d431
Compare
cbae15b to
80be943
Compare
ceaefd0 to
9e3c1e4
Compare
Signed-off-by: Jie-Kai Chang <jiekai.chang326@gmail.com>
Signed-off-by: 400Ping <jiekai.chang326@gmail.com>
Signed-off-by: 400Ping <jiekai.chang326@gmail.com>
Future-Outlier
left a comment
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.
Hi, @400Ping
do you mind solve the merge conflict? thank you!
also cc @fsNick @win5923 @JiangJiaWei1103 for review
Signed-off-by: Jie-Kai Chang <jiekai.chang326@gmail.com>
| PartitionID: "aws", | ||
| URL: endpoint, | ||
| SigningRegion: "us-east-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.
Thanks for contributing! Would you mind defining the partition and the region as constants so they can be reused?
For example:
config.WithRegion("us-east-1"),here can be modified to:
config.WithRegion(s3Region),
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.
done in 54764d6
Signed-off-by: 400Ping <jiekai.chang326@gmail.com>
Signed-off-by: 400Ping <jiekai.chang326@gmail.com>
historyserver/pkg/storage/s3/s3.go
Outdated
| awsconfig.WithHTTPClient(httpClient), | ||
| } | ||
| if endpoint != "" { | ||
| resolver := aws.EndpointResolverWithOptionsFunc( |
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.
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.
Thanks for the catch, updated in e0962df
Signed-off-by: 400Ping <jiekai.chang326@gmail.com>
Signed-off-by: Jie-Kai Chang <jiekai.chang326@gmail.com>
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.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Signed-off-by: 400Ping <jiekai.chang326@gmail.com>
Signed-off-by: 400Ping <jiekai.chang326@gmail.com>

Why are these changes needed?
Summary
Migrates the historyserver S3 client from AWS SDK v1 to v2, covering client initialization, error handling, pagination, and MinIO path‑style behavior, plus updates the e2e tests and module dependencies.
Changes
s3.go: switch to v2 config/client, paginator, and smithy error handling; keep bucket auto‑create and MinIO path‑style.config.go: remove v1 aws.Bool usage; use bool + parsing helpers.collector_test.go: update e2e S3 client/paginator/types to v2.Related issue number
Closes #4280
Checks