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.

Sign up for GitHub

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

Jump to bottom

Update session control inside pool #503

Merged
merged 2 commits into from
Aug 30, 2023
Merged

Update session control inside pool #503

merged 2 commits into from
Aug 30, 2023

Conversation

smallhive
Copy link
Contributor

closes #499

@smallhive
tests: Make epoch changing configurable
3c53308 
Signed-off-by: Evgenii Baidakov <evgenii@nspcc.io>
@smallhive smallhive marked this pull request as ready for review August 30, 2023 07:48
carpawell
carpawell reviewed Aug 30, 2023
View reviewed changes
Copy link
Member

@carpawell carpawell left a comment

Choose a reason for hiding this comment

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

Usually, I expect some commit body that explains + 1 changes.

@roman-khimov
Copy link
Member

Yeah, this needs a real explanation of the mechanics behind it and intended consequences.

@smallhive smallhive force-pushed the 499-session-token-not-found branch 2 times, most recently from d57690f to d7ac14f Compare August 30, 2023 08:41
@smallhive
Copy link
Contributor Author

Extended commit message text. Any suggestions are welcome

@smallhive
pool: Expire session token 1 epoch earlier
2c705b5 
Expire session one epoch earlier to avoid situation when token already expired,
but still exists in poll's cache. It may happen because epoch changed, but pool
not updated yet. This leads to errors "session token not found" or "expired session token".
Now, when using ExpiredAt(epoch + 1), the system ensures that at each epoch, it evaluates
the upcoming one for token expiration, rather than evaluating the current one.

closes #499

Signed-off-by: Evgenii Baidakov <evgenii@nspcc.io>
AliceInHunterland
AliceInHunterland approved these changes Aug 30, 2023
View reviewed changes
Copy link

@AliceInHunterland AliceInHunterland left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM

carpawell
carpawell approved these changes Aug 30, 2023
View reviewed changes
Copy link
Member

@carpawell carpawell left a comment

Choose a reason for hiding this comment

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

It may happen because epoch changed, but pool
not updated yet.

How often the Pool is updated? Can it be less than once an epoch?

@roman-khimov
Copy link
Member

Can it be less than once an epoch?

Technically can. Practically highly unlikely.

@roman-khimov roman-khimov merged commit 31cf1bc into master Aug 30, 2023
5 checks passed
@roman-khimov roman-khimov deleted the 499-session-token-not-found branch August 30, 2023 13:31
@vvarg229 vvarg229 mentioned this pull request Aug 31, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AliceInHunterland AliceInHunterland AliceInHunterland approved these changes

@carpawell carpawell carpawell approved these changes

@roman-khimov roman-khimov roman-khimov approved these changes

@cthulhu-rider cthulhu-rider Awaiting requested review from cthulhu-rider cthulhu-rider is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

PutObjectRetention and other s3 calls fails with session token not found
4 participants
@smallhive @roman-khimov @carpawell @AliceInHunterland