-
Notifications
You must be signed in to change notification settings - Fork 990
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
Remove blpop FindFirst hop after wakeup #1168
Conversation
Signed-off-by: Vladislav Oleshko <[email protected]>
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.
Can you also verify with ruby/sidekick test from our playground repo that everything is working well and nothing deadlocks/crashes?
src/server/transaction.cc
Outdated
// We don't care about preserving the strict order with multiple operations running on blocking | ||
// keys in parallel, because the internal order is not observable from outside either way. | ||
uint64_t default_txid = UINT64_MAX; | ||
if (!notify_txid_.compare_exchange_weak(default_txid, committed_txid, memory_order_relaxed)) |
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.
compare_exchange_weak is an overkill for "run-once" semantics.
Also, I wonder if notify_txid_
is needed after this change or it's been used only here
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.
Indeed, I forgot to simplify FindFirst.
compare_exchange_weak is an overkill for "run-once" semantics.
How else should I do run-once? 👀
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.
for example,
if (var.fetch_add(1, relaxed) == 0) {
}
Signed-off-by: Vladislav Oleshko <[email protected]>
Signed-off-by: Vladislav Oleshko <[email protected]>
Signed-off-by: Vladislav Oleshko <[email protected]>
I checked the ruby playground and it seems to work (worker rb + loadtest) |
Solves #1153
Adds the following changes: