Skip to content

Pass null-terminated node ID for VM_RegisterClusterMessageReceiver and add test coverage#1708

Merged
zuiderkwast merged 12 commits into
valkey-io:unstablefrom
Nikhil-Manglore:module_callback_registration
Feb 20, 2025
Merged

Pass null-terminated node ID for VM_RegisterClusterMessageReceiver and add test coverage#1708
zuiderkwast merged 12 commits into
valkey-io:unstablefrom
Nikhil-Manglore:module_callback_registration

Conversation

@Nikhil-Manglore

@Nikhil-Manglore Nikhil-Manglore commented Feb 11, 2025 •
edited by enjoy-binbin
Loading

Copy link
Copy Markdown
Member

Closes #1656.

@Nikhil-Manglore
Added test coverage for module callback registration for cluster message
bc17955 
Signed-off-by: Nikhil Manglore <nmanglor@amazon.com>
@codecov

codecov Bot commented Feb 11, 2025 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 0% with 5 lines in your changes missing coverage. Please review.

Project coverage is 71.14%. Comparing base (da3f1c6) to head (18117ba).
Report is 37 commits behind head on unstable.

Files with missing lines Patch % Lines
src/cluster_legacy.c 0.00% 3 Missing ⚠️
src/module.c 0.00% 2 Missing ⚠️
Additional details and impacted files 
@@             Coverage Diff              @@
##           unstable    #1708      +/-   ##
============================================
+ Coverage     71.06%   71.14%   +0.08%     
============================================
  Files           121      123       +2     
  Lines         65254    65534     +280     
============================================
+ Hits          46371    46627     +256     
- Misses        18883    18907      +24
Files with missing lines Coverage Δ
src/module.c 9.61% <0.00%> (+0.01%) ⬆️
src/cluster_legacy.c 85.87% <0.00%> (-0.39%) ⬇️

... and 32 files with indirect coverage changes

@Nikhil-Manglore

Nikhil-Manglore commented Feb 11, 2025

Copy link
Copy Markdown
Member Author

Looking into the CI Failure

@Nikhil-Manglore
Fixed CI Failure where two messages were expected due to Ping/Pong fu…
0699352 
…nctionality

Signed-off-by: Nikhil Manglore <nmanglor@amazon.com>
@Nikhil-Manglore
Reverting number of messages sent back to 1
ccfba46 
Signed-off-by: Nikhil Manglore <nmanglor@amazon.com>
hpatro
hpatro reviewed Feb 11, 2025
View reviewed changes
Comment thread tests/modules/cluster.c Outdated
@Nikhil-Manglore
Removed counter increments to fix CI failure
4e97d19 
Signed-off-by: Nikhil Manglore <nmanglor@amazon.com>
hpatro
hpatro reviewed Feb 12, 2025
View reviewed changes
Comment thread tests/modules/cluster.c Outdated
Comment thread tests/unit/moduleapi/cluster.tcl Outdated
@Nikhil-Manglore
Sent acknowledgement response back to the sender
2a12076 
Signed-off-by: Nikhil Manglore <nmanglor@amazon.com>
@Nikhil-Manglore
Fixed formatting error
c2d596c 
Signed-off-by: Nikhil Manglore <nmanglor@amazon.com>
hpatro
hpatro reviewed Feb 14, 2025
View reviewed changes
Comment thread src/cluster_legacy.c Outdated
@Nikhil-Manglore
Used SDS instead of manual string truncation and refactored code
d5938c5 
Signed-off-by: Nikhil Manglore <nmanglor@amazon.com>
hpatro
hpatro reviewed Feb 17, 2025
View reviewed changes

@hpatro hpatro left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly LGTM.

Comment thread src/cluster_legacy.c Outdated
Comment thread tests/unit/moduleapi/cluster.tcl
Comment thread tests/unit/moduleapi/cluster.tcl Outdated
@Nikhil-Manglore
Added extra verification of log messages
3d12b72 
Signed-off-by: Nikhil Manglore <nmanglor@amazon.com>
@hpatro hpatro requested a review from zuiderkwast February 17, 2025 20:37
enjoy-binbin
enjoy-binbin reviewed Feb 18, 2025
View reviewed changes
Comment thread src/cluster_legacy.c
@Nikhil-Manglore Nikhil-Manglore changed the title Added test coverage for module callback registration for cluster message Null terminated the Node ID and added test coverage for module callback registration for cluster message Feb 19, 2025
@Nikhil-Manglore Nikhil-Manglore changed the title Null terminated the Node ID and added test coverage for module callback registration for cluster message Null-terminated Node ID and add test coverage for module cluster message callbacks Feb 19, 2025
@Nikhil-Manglore
Updated documentation
99ce31b 
Signed-off-by: Nikhil Manglore <nmanglor@amazon.com>
@hpatro hpatro changed the title Null-terminated Node ID and add test coverage for module cluster message callbacks Pass null-terminated node ID for VM_RegisterClusterMessageReceiver and add test coverage Feb 19, 2025
hpatro
hpatro reviewed Feb 19, 2025
View reviewed changes
Comment thread src/module.c Outdated
@Nikhil-Manglore
Improved documentation
4d3c079 
Signed-off-by: Nikhil Manglore <nmanglor@amazon.com>
zuiderkwast
zuiderkwast reviewed Feb 19, 2025
View reviewed changes
Comment thread src/module.c Outdated
zuiderkwast
zuiderkwast reviewed Feb 19, 2025
View reviewed changes

@zuiderkwast zuiderkwast left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, just some nits, then let's merge it.

Comment thread src/cluster_legacy.c Outdated
Comment thread tests/unit/moduleapi/cluster.tcl Outdated
@Nikhil-Manglore
Updated documentation based on suggestions
6e47324 
Signed-off-by: Nikhil Manglore <nmanglor@amazon.com>
@Nikhil-Manglore
Fixed suggestions
18117ba 
Signed-off-by: Nikhil Manglore <nmanglor@amazon.com>
@zuiderkwast zuiderkwast merged commit 206f7f6 into valkey-io:unstable Feb 20, 2025
@Nikhil-Manglore Nikhil-Manglore deleted the module_callback_registration branch February 20, 2025 00:25
madolson
madolson reviewed Feb 21, 2025
View reviewed changes
Comment thread tests/unit/moduleapi/cluster.tcl
@hpatro hpatro mentioned this pull request Feb 21, 2025
Closed
zuiderkwast pushed a commit that referenced this pull request Mar 18, 2025
@Nikhil-Manglore @zuiderkwast
Pass null-terminated node ID for VM_RegisterClusterMessageReceiver …
734c0df 
…and add test coverage (#1708)

* Pass `sender_id` as `NULL` terminated string as part of
`ValkeyModuleClusterMessageReceiver` for ease of usage by the module(s).

* Implement test coverage described in #1656 and ensures that nodes in
the cluster module properly acknowledge a "DING" message by sending a
"DONG" response.

Closes #1656.

---------

Signed-off-by: Nikhil Manglore <nmanglor@amazon.com>
xbasel pushed a commit to xbasel/valkey that referenced this pull request Mar 27, 2025
@Nikhil-Manglore @xbasel
Pass null-terminated node ID for VM_RegisterClusterMessageReceiver …
0c8409b 
…and add test coverage (valkey-io#1708)

* Pass `sender_id` as `NULL` terminated string as part of
`ValkeyModuleClusterMessageReceiver` for ease of usage by the module(s).

* Implement test coverage described in valkey-io#1656 and ensures that nodes in
the cluster module properly acknowledge a "DING" message by sending a
"DONG" response.

Closes valkey-io#1656.

---------

Signed-off-by: Nikhil Manglore <nmanglor@amazon.com>
xbasel pushed a commit to xbasel/valkey that referenced this pull request Mar 27, 2025
@Nikhil-Manglore @xbasel
Pass null-terminated node ID for VM_RegisterClusterMessageReceiver …
3b00461 
…and add test coverage (valkey-io#1708)

* Pass `sender_id` as `NULL` terminated string as part of
`ValkeyModuleClusterMessageReceiver` for ease of usage by the module(s).

* Implement test coverage described in valkey-io#1656 and ensures that nodes in
the cluster module properly acknowledge a "DING" message by sending a
"DONG" response.

Closes valkey-io#1656.

---------

Signed-off-by: Nikhil Manglore <nmanglor@amazon.com>
@zuiderkwast zuiderkwast moved this from To be backported to Unknown if backported in Valkey 8.1 Apr 7, 2025
murphyjacob4 pushed a commit to enjoy-binbin/valkey that referenced this pull request Apr 13, 2025
@Nikhil-Manglore @murphyjacob4
Pass null-terminated node ID for VM_RegisterClusterMessageReceiver …
bcf37ec 
…and add test coverage (valkey-io#1708)

* Pass `sender_id` as `NULL` terminated string as part of
`ValkeyModuleClusterMessageReceiver` for ease of usage by the module(s).

* Implement test coverage described in valkey-io#1656 and ensures that nodes in
the cluster module properly acknowledge a "DING" message by sending a
"DONG" response.

Closes valkey-io#1656.

---------

Signed-off-by: Nikhil Manglore <nmanglor@amazon.com>
@zuiderkwast zuiderkwast moved this from Unknown if backported to Done in Valkey 8.1 Apr 17, 2025
@ranshid ranshid mentioned this pull request Aug 5, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@enjoy-binbin enjoy-binbin enjoy-binbin left review comments
@hpatro hpatro hpatro approved these changes
@madolson madolson madolson left review comments
@zuiderkwast zuiderkwast zuiderkwast approved these changes

Assignees

No one assigned

Labels

None yet

Projects

Valkey 8.1
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add test coverage for module callback registration for cluster message via VM_RegisterClusterMessageReceiver

5 participants

@Nikhil-Manglore @zuiderkwast @enjoy-binbin @hpatro @madolson