-
Notifications
You must be signed in to change notification settings - Fork 702
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
Enable module API SendClusterMessage to use light message header type #1572
base: unstable
Are you sure you want to change the base?
Enable module API SendClusterMessage to use light message header type #1572
Conversation
Signed-off-by: Harkrishn Patro <[email protected]>
Signed-off-by: Harkrishn Patro <[email protected]>
Signed-off-by: Harkrishn Patro <[email protected]>
@@ -195,11 +195,13 @@ dictType clusterSdsToListType = { | |||
typedef struct { | |||
enum { | |||
ITER_DICT, | |||
ITER_LIST | |||
ITER_LIST, | |||
ITER_NODE, |
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 helps simplify the code a bit however it's just iterating over one node.
Signed-off-by: Harkrishn Patro <[email protected]>
Signed-off-by: Harkrishn Patro <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #1572 +/- ##
============================================
- Coverage 70.94% 70.94% -0.01%
============================================
Files 121 121
Lines 65130 65181 +51
============================================
+ Hits 46208 46240 +32
- Misses 18922 18941 +19
|
Signed-off-by: Harkrishn Patro <[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.
It looks pretty good, but we need cross-version testing IMO with a mixed cluster to make sure it works during rolling upgrade, i.e. a mixed cluster. Can you take my work from #1371 and add it here? Or help completing it.
@zuiderkwast just out of curiosity what is pending over here? I see @madolson approved the PR as well. I can help contribute / complete it. |
This change uses the light message header for cluster module message type to be sent to a target node or broadcast across the cluster. The light message header was introduced in Valkey 8 to reduce network in/out over clusterbus and have been already used for pub/sub message transfer. It's only used if the target node supports the light message header (~30B) otherwise it falls back to using the regular header (~2KB). The module API
VM_SendClusterMessage
remains as is.