-
Notifications
You must be signed in to change notification settings - Fork 761
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
Fix engine crash on module client blocking during keyspace events #1819
base: unstable
Are you sure you want to change the base?
Conversation
a064f39
to
74cb5e4
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #1819 +/- ##
============================================
- Coverage 70.99% 70.98% -0.01%
============================================
Files 123 123
Lines 65651 65728 +77
============================================
+ Hits 46609 46660 +51
- Misses 19042 19068 +26
🚀 New features to boost your workflow:
|
2428ae0
to
7c9cff2
Compare
dd60c6a
to
0445d46
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.
Ok, I'm aligned with the design now. Just some other misc comments.
Co-authored-by: Madelyn Olson <[email protected]> Signed-off-by: Yair Gottdenker <[email protected]>
Co-authored-by: Madelyn Olson <[email protected]> Signed-off-by: Yair Gottdenker <[email protected]> Signed-off-by: yairgott <[email protected]>
@valkey-io/core-team This API is a bit late to be added. The feature adds a new functionality to using keyspace notifications, allowing them to block clients. The goal is to provide read and after write consistency for the VSS module. There are two different ways to provide this guarantee, either block the write until it's visible in the index or block the read until all pending writes were ingested. The search module preferred to use the blocking of the write approach, but need an API to block the writes. @murphyjacob4 Will follow up to see if there is a away to avoid this new functionality. |
Co-authored-by: Madelyn Olson <[email protected]> Signed-off-by: Yair Gottdenker <[email protected]> Signed-off-by: yairgott <[email protected]>
Note: this PR is mainly fixing an engine crash and read after write consistency on the same connection related to the VSS module, so it is technically a bug fix in my mind, not new feature. Also with the latest revision, the module API change is kept to minimal. |
Signed-off-by: yairgott <[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.
I didn't review the test case yet. It's failing though in the latest CI run:
*** [err]: Event that fires twice in tests/unit/moduleapi/block_keyspace_notification.tcl
Rename event not propagated within 5 seconds
Please look into it. We've had many problems with flaky test.
Signed-off-by: yairgott <[email protected]>
Signed-off-by: yairgott <[email protected]>
Signed-off-by: yairgott <[email protected]>
With the simplified module API code, I will take another look this week on the functional change in |
…VM_UnblockClient Signed-off-by: yairgott <[email protected]>
Signed-off-by: yairgott <[email protected]>
Signed-off-by: yairgott <[email protected]>
Signed-off-by: yairgott <[email protected]>
This change enhances user experience and consistency by allowing a module to block a client on keyspace event notifications. Consistency is improved by allowing that reads after writes on the same connection yield expected results. For example, in ValkeySearch, mutations processed earlier on the same connection will be available for search.
The implementation extends
VM_BlockClient
to support blocking clients on keyspace event notifications. Internal clients, LUA clients, clients issueing multi exec and those with thedeny_blocking
flag set are not blocked. Once blocked, a client’s reply is withheld until it is explicitly unblocked.