Docstore/awsdynamodb: Supporting the AWS V2 SDK #3519
Conversation
…erences to aws sdk v2
…zation and changed error type
…es, returning that concrete type to avoid unnecessary type assertion
…eatable outside of CI
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
We appreciate the effort you put into this. However, there is a lot of changed code here, including at least two breaking changes. How does the docstore user benefit from the switch to the new API?
Note that the breaking changes are unlikely to be approved, because they would require a v2 of the main module.
| @@ -36,6 +37,7 @@ aws dynamodb create-table \ | |||
| # The docstore-test-2 table has both a partition and a sort key, and two indexes. | |||
|
|
|||
| aws dynamodb create-table \ | |||
| --region us-east-2 \ | |||
There was a problem hiding this comment.
The unit tests hard code the region as us-east-2, but the creation script used to set up the test db's don' t specify a region, falling back to the user default, this was to improve consistency in testing
| @@ -75,8 +69,6 @@ const Scheme = "dynamodb" | |||
| // See https://godoc.org/gocloud.dev/aws#ConfigFromURLParams for supported query | |||
| // parameters for overriding the aws.Session from the URL. | |||
| type URLOpener struct { | |||
| // ConfigProvider must be set to a non-nil value. | |||
There was a problem hiding this comment.
This was flagged in the original message soliciting feedback on the best way to address, as omitting it is in keeping with several other V2 implementations, but regardless the type will change to support V2
| if err != nil { | ||
| return nil, "", "", "", nil, fmt.Errorf("open collection %s: %v", u, err) | ||
| } | ||
| return db, tableName, partitionKey, sortKey, opts, nil | ||
| } | ||
|
|
||
| // Dial gets an AWS DynamoDB service client. | ||
| func Dial(p client.ConfigProvider) (*dyn.DynamoDB, error) { | ||
| if p == nil { | ||
| func Dial(p aws.Config) (*dyn.Client, error) { |
There was a problem hiding this comment.
This has changed to the equivalent type used for AWS SDK V2, again, please see the comment below on why that change has already been discussed and is necessary.
|
Hi Johnathan, Last year the AWS SDK V1 was deprecated (see announcement about maintenance and end of support in project readme) with its maintenance grace period set to end on 7/31/2025, this PR was created to address that and move the package to using the new SDK as all the other modules already have. This was discussed in the flagged issue #3458
I'll be adding comments on the three sections you've specifically flagged as well. |
Resolved #3458
A somewhat quick and dirty conversion to using the AWS V2 SDK rather than the deprecated V1.
Due to the delays in my finding time to work on this I prioritised getting the functionality migrated to mitigate any risk from API drift form the deprecated V1 SDK.
The main sections that warrant further discussion:
V2ConfigFromURLParams, but is worth review