-
Notifications
You must be signed in to change notification settings - Fork 219
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
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
ClusterState
was keeping HashMap of session_id -> SessionContext which was never cleaned up.ClusterState
will never emit events about accessed or removedSessionContexts
.ClusterState
and emit events when cache expires orSessionContext
events and react whensession_id
times out (keep cache out ofClusterState
.SessionContext
is authoritative not the context kept scheduler, it will simplify client scheduler interaction if initialcreate_context
is skipped.ClusterState
is 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_context
call.get_context
has been replaced withcreate_or_update_context
SessionContext
notifications are published (SessionAccessed
,SessionRemoved
)SessionAccessed
,SessionRemoved
addedSessionUpdate
removedcreate_or_update_context
method changesession_id
is required to be setoperation_id
added to help with client-scheduler request mappingAre there any user-facing changes?
To be done