-
Notifications
You must be signed in to change notification settings - Fork 303
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
Quick ActorCache refactor #1914
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.
This set of changes LGTM modulo the TODO(now) in the Make Entry::syncStatus private
commit. If you want to get it in ahead of the other PRs, feel free.
5fa38e9
to
d7857a7
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.
LGTM
fcef6f6
to
cb4562c
Compare
Reading actor cache isn't easy, and if things are immutable then we should convey that in the code.
This allows us to mutate from UNKNOWN to PRESENT or ABSENT but permit no other transition.
This commit introduces `addToCleanList()` and `addToDirtyList()` to make it obvious when we're actually transitioning between lists.
f8b58b5
to
d680fba
Compare
This helps make it a bit more obvious that we don't want to touch `syncStatus` directly.
d680fba
to
3b5e7a0
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.
Still LGTM, but it looks like merge on this will be blocked until @justin-mp removes his "Request changes" bit.
These commits were scattered before and after the larger read coalesce changes, and since that change is really big and complicated, I figured it would be easier to yank these quick refactors out into a separate PR so we can focus on the scary stuff elsewhere.