Skip to content

Conversation

Jonathan-Rosenberg
Copy link
Contributor

@Jonathan-Rosenberg Jonathan-Rosenberg commented Sep 17, 2025

Closes #9428

In addition to making it configurable, change the default expiration window to 60 seconds (instead of 30) to fix the bug mentioned above (read my comment in the issue for more details).

Created an issue in the AWS Go SDK repo

The AWS SDK bug was actually fixed in a later version. I've upgraded the packages to include the fix.

…iry window from 30 seconds to 60 seconds (in order to avoid retry backoff and expiry times mix)
@Jonathan-Rosenberg Jonathan-Rosenberg added bug Something isn't working include-changelog PR description should be included in next release changelog labels Sep 17, 2025
@Jonathan-Rosenberg Jonathan-Rosenberg requested a review from a team September 17, 2025 10:26
Copy link

github-actions bot commented Sep 17, 2025

📚 Documentation preview at https://pr-9527.docs-lakefs-preview.io/

(Updated: 9/21/2025, 1:19:15 PM - Commit: fbbcc50)

Copy link
Contributor

@nopcoder nopcoder left a comment

Choose a reason for hiding this comment

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

Looks good; just missing unit in the jitter parameter name.

Comment on lines 133 to 135
// CredentialsCacheExpiryWindowJitter - The jitter fraction for credentials cache expiry.
// Default is 0.5 (50% jitter).
CredentialsCacheExpiryWindowJitter float64 `mapstructure:"credentials_cache_expiry_window_jitter"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Add 'Frac' as suffix. it will be hard to understand the unit is not in duration.

Copy link
Contributor

Choose a reason for hiding this comment

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

"Fraction", please: "frac" is 4 characters shorter but harder to understand. Or (I prefer) "proportion"; just let's not abbreviate here.

@lucix-aws
Copy link

FYI aws/aws-sdk-go-v2#3192 (comment)

@N-o-Z
Copy link
Member

N-o-Z commented Sep 17, 2025

@Jonathan-Rosenberg are these changes relevant considering the investigation you made and the suggestion from AWS to update the dynamodb package?
Perhaps we should wait with these and first update the package?

@Jonathan-Rosenberg
Copy link
Contributor Author

@N-o-Z just seen the comment now. Amazing so I'll update it. Anyways, I still think these should be configurable and not constant, but I'll definitly try to update it first.

Copy link
Contributor

@arielshaqed arielshaqed left a comment

Choose a reason for hiding this comment

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

Neat, thanks!

But really cool that newer versions actually have the fix you wanted 😎

Comment on lines 133 to 135
// CredentialsCacheExpiryWindowJitter - The jitter fraction for credentials cache expiry.
// Default is 0.5 (50% jitter).
CredentialsCacheExpiryWindowJitter float64 `mapstructure:"credentials_cache_expiry_window_jitter"`
Copy link
Contributor

Choose a reason for hiding this comment

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

"Fraction", please: "frac" is 4 characters shorter but harder to understand. Or (I prefer) "proportion"; just let's not abbreviate here.

@Jonathan-Rosenberg Jonathan-Rosenberg merged commit 75eabde into master Sep 21, 2025
43 checks passed
@Jonathan-Rosenberg Jonathan-Rosenberg deleted the feature/dynamodb/set-creds-expiry-window branch September 21, 2025 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working include-changelog PR description should be included in next release changelog

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Possible issue in dynamoDB KV store when receiving slow down

6 participants