Skip to content
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

Fix unreleased capacity after retryToken is requested #1424

Closed
wants to merge 1 commit into from

Conversation

Skarlso
Copy link

@Skarlso Skarlso commented Sep 18, 2021

Closes #1413

For further details, please read the attached issue.

One issue I could still see possibly is that this is always overwritten and only called ONCE after a success. I'm not familiar with the code enough that after a success the Attempt and the retry is re-created completely? Because then this could be fine.

if len(e.Results) != len(a.Results) {
t.Fatalf("expected len %d got %d", len(e.Results), len(a.Results))
}
for i, eRes := range e.Results {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a side effect of altering the return value with adding a function into it. Can no longer be deepEqualed. If anyone has a nice idea on how to fix that, I'm open for suggestions.

Sadly, it's late and I can't think of anything better ATM. :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can experiment with a mock retry provider in these cases.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be able to change how these values are compared to use cmp.Diff. This is commonly used for other data types in the v2 SDK.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can try that, sure.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though I'm not sure how that works for functions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clarifying that. Yes, you're correct cmp.Diff doens't work with functions, checking for not-nil is pretty much the only way to compare in this case.

jasdel added a commit to jasdel/aws-sdk-go-v2 that referenced this pull request Jan 14, 2022
Updates the Retry middleware to release the retry token, on subsequent
attempts. This fixes aws#1413, and is based on PR aws#1424.
jasdel added a commit to jasdel/aws-sdk-go-v2 that referenced this pull request Jan 14, 2022
Updates the Retry middleware to release the retry token, on subsequent
attempts. This fixes aws#1413, and is based on PR aws#1424.
@jasdel
Copy link
Contributor

jasdel commented Jan 14, 2022

Thanks for taking the time to create this PR @Skarlso. I've taken these changes and incorporated them in #1560. That PR takes the idea of these changes, but instead of using a member on AttemptResults updates handleAttempt to take in the previous releaseRetryToken, and return the next token. With nopRelease (No-Op func) being return if there is no retry token.

Lets use #1560 to get this changed merged in, and released into the SDK.

@jasdel jasdel closed this Jan 14, 2022
@Skarlso
Copy link
Author

Skarlso commented Jan 14, 2022

@jasdel Nice improvement. :)

jasdel added a commit that referenced this pull request Jan 14, 2022
Updates the Retry middleware to release the retry token, on subsequent
attempts. This fixes #1413, and is based on PR #1424.
jrichard8 pushed a commit to jrichard8/aws-sdk-go-v2 that referenced this pull request Feb 14, 2022
Updates the Retry middleware to release the retry token, on subsequent
attempts. This fixes aws#1413, and is based on PR aws#1424.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

aws/retry: Retry token post request attempt not being cleared
2 participants