-
-
Notifications
You must be signed in to change notification settings - Fork 178
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
Migrate to AWS SDK v2 #206
Conversation
* using sdk-v2
The official API doesn't accept int64 limits.
* Recover DYNAMO_TEST_REGION * Error handling * No extra type
I'm looking forward to this update. |
Thanks! I'll take a look at this then merge it into a new dev for v2 branch. Maybe the (Tx idempotency) UUID thing can be solved by switching our UUID generation strategy. |
Hi @guregu how is the progress on this? Would be nice to hear that in the near future we might see an upgrade here :) |
Sorry it's taken so long. It's a high priority for me at the moment. I'll try and get a beta branch for v2 up Soon™ (maybe sometime this month?). |
Looking forward for the v2 support, would help to remove quite some aws v1 stuff from the codebase. ❤️ |
@guregu btw I noticed something this week. When using ec2 role based credentials, dynamodb would give me a serialization issue when using guregu. Maybe you need to tweak something there. No time to identify the issue completely here. For our SQS client no problem, only for dynamodb. So I highly suspect that there is some bug here |
In my experience, that happens when you have a primary key that is empty (like an empty string). awsCfg := &aws.Config{
LogLevel: aws.LogLevel(aws.LogDebugWithHTTPBody),
}
dynamoDB := dynamo.New(session.New(), awsCfg) The error message is a bug though, we should fix it to give a better error instead. Please open an issue if you find something weird 👀 |
@PauPauRob The latest release (v1.18.2) adds some checking against the missing PK thing I was talking about, see if it helps. |
Hello. Anything we can contribute here? @guregu |
FYI, there's a fork here https://github.com/junderhill/helixddb that uses v2 (based on this PR). I still plan on adding v2 support, but there are a few things that we should do first. One of those is decide about whether we want to change the way the builder objects work. See: #213. Have not got any feedback yet. I'm also curious about what part of v2 people find most important. Is it supporting the marshaling interfaces for v2? Sharing config? Binary sizes? Maybe we can make something work with both without bloating binaries (maybe not). |
To be totally honest I want this using V2 only because I have all the latest dependencies in my project and I can't use this library because of old dependencies conflicts. |
This was the exact reason I decided to fork the project - we had dependencies on multiple v2 AWS packages already. If it helps, we've been using my fork in prod at Serif for a short while and haven't had any issues. |
I made a fork of this PR myself and I'm using the exact code in this PR... So far so good |
@guregu I would humbly encourage you not to make aws sdk v2 support contingent on dynamo v2 (which I take is your current thinking based on your reference to #213). The v1 aws go sdk has been considered superseded if not deprecated for some time now and has been very slow to get updates including basic usability ones like support for sso_session configuration. I would also +1 several of the above reasons for needing sdkv2 support. I can add my voice to the "we've been using this fork and it seems fine" chorus, so given the lack of support here is starting to proliferate forks I would argue it would be good to just get this merged and get everyone back on the upstream. If you were thinking of implementing it differently for dynamo v2 you can still do that, but merging this gets sdkv2 support unblocked while you work on v2. Thanks for all your work on dynamo ❤️ |
As it happens I believe we've been hit by aws/aws-sdk-go-v2#2370, so personally I've had to fork @irohiroki's branch to bump the version of the sdkv2 packages again |
Hello again everyone. Thanks for the feedback and patience.
|
Hello everyone. Just wanted to post a little update. The changes to v1 I wanted to make should be done now. I will do my best to get a beta branch for v2 up before the end of the year. Happy holidays. |
Good news everyone. I have this PR merged in https://github.com/guregu/dynamo/tree/v2-dev. It's mostly working but there's an issue with TestTxRetry. The rest of the tests are passing though. I'll continue poking at it. |
Fixed this, v2-dev branch is passing all the tests now. Also removed the remaining dependencies on the v1 SDK. |
Hello everyone, I made a new PR here: #232. It includes all the changes from here, of course, just with the conflicts fixed. |
Migration to AWS SDK v2.
This PR is based on @niltonkummer's work, and I made modifications to pass tests and finish as a PR to the main stream. This version is working fine with my own application (a DB performance test tool).
some notes: