-
Notifications
You must be signed in to change notification settings - Fork 261
feat: ClusterState does not cache session contexts
#1226
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
ClusterState does not cache session contexts
9902f25 to
c9ea189
Compare
9fcc709 to
01534b4
Compare
andygrove
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't currently have time to review large PRs like this unfortunately, but I skimmed through it and it looks reasonable.
... as session context was not cleaned up.
29434b1 to
cee1510
Compare
... as session context was not cleaned up.
Which issue does this PR close?
Closes #1220.
Rationale for this change
ClusterStatewas keeping HashMap of session_id -> SessionContext which was never cleaned up.ClusterStatewill never emit events about accessed or removedSessionContexts.ClusterStateand emit events when cache expires orSessionContextevents and react whensession_idtimes out (keep cache out ofClusterState.SessionContextis authoritative not the context kept scheduler, it will simplify client scheduler interaction if initialcreate_contextis skipped.ClusterStateis stateless from connected client perspectiveWhat changes are included in this PR?
In order to have expiring session contexts a LRU cache is needed, which I did not want to bring in. So this implementation proposes not to cache session contexts, rather to create them every time. This is trade off between correctness and keeping additional dependency in. It can be revisited later if needed.
Also, this change brings:
create_contextcall.get_contexthas been replaced withcreate_or_update_contextSessionContextnotifications are published (SessionAccessed,SessionRemoved)SessionAccessed,SessionRemovedaddedSessionUpdateremovedcreate_or_update_contextmethod changesession_idis required to be setoperation_idadded to help with client-scheduler request mappingAre there any user-facing changes?
To be done