Skip to content
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

Open
wants to merge 6 commits into
base: unstable
Choose a base branch
from

Conversation

hpatro
Copy link
Collaborator

@hpatro hpatro commented Jan 16, 2025

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.

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,
Copy link
Collaborator Author

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]>
src/cluster_legacy.c Outdated Show resolved Hide resolved
Copy link

codecov bot commented Jan 17, 2025

Codecov Report

Attention: Patch coverage is 18.75000% with 52 lines in your changes missing coverage. Please review.

Project coverage is 70.94%. Comparing base (87cc3d7) to head (5322d99).
Report is 1 commits behind head on unstable.

Files with missing lines Patch % Lines
src/cluster_legacy.c 18.75% 52 Missing ⚠️
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     
Files with missing lines Coverage Δ
src/module.c 9.60% <ø> (ø)
src/cluster_legacy.c 86.22% <18.75%> (-0.97%) ⬇️

... and 11 files with indirect coverage changes

Signed-off-by: Harkrishn Patro <[email protected]>
@hpatro hpatro requested a review from a team January 21, 2025 20:53
Copy link
Contributor

@zuiderkwast zuiderkwast left a 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.

@sarthakaggarwal97
Copy link
Contributor

sarthakaggarwal97 commented Jan 22, 2025

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants