-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Clear sessions cache on transfer #5608
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
base: master
Are you sure you want to change the base?
Conversation
3efc072
to
e056045
Compare
|
:pass | ||
end | ||
else | ||
unlock(name) |
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.
That's a bit of a weak point here. The lock_timeout
ensures the buildup of queues in write buffers is not too big. However, if transfer gets stuck for whatever reason or takes longer than the timeout, the writebuffers will resume processing events.
One way to go about this could be wiping cache here before unlock. I'm not sure, however, whether it won't break consistency of transferred cache if the wipe happens right in the middle of transfer. Likely not, as :ets.delete_all_objects
is supposedly atomic, but I'm not 100% certain about how table dumping works with deletion in the middle of :ets.tab2list
.
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 have changed locking approach - we only need to lock Session.WriteBuffer for the duration of the transfer, no need to block both queues.
Also the session cache is wiped when the lock times out.
|
||
Process.flag(:trap_exit, true) | ||
timer = Process.send_after(self(), :tick, flush_interval_ms) | ||
|
||
^name = :ets.new(name, [:named_table, :set, :public]) |
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.
do we need an ets table to hold the lock? would it be possible to keep it in the process state? No loop necessary either. Or am I reading this wrong?
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.
The problem is, process state can only be accessed from within the process. While locking could be done with message passing, unlocking requires doing that outside the process queue which holds all the pending events to process stuck there for the duration of the lock. Unless you know some other way? 😅
239b348
to
f6416b0
Compare
Changes
This PR implements clearing session cache on complete transfer in order to avoid making the already transferred session stale. To further prevent this, both, session and event WriteBuffer processes are blocked for the duration of the transfer.
The feature is behind an ENV feature flag,
CLICKHOUSE_SESSION_LOCK_ENABLED
which defaults tofalse
. Enabling it will require adjusting config on infra level.Tests