Enable module API SendClusterMessage to use light message header type

[hpatro]

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.

[codecov]

Codecov Report

Attention: Patch coverage is 14.66667% with 64 lines in your changes missing coverage. Please review.

Project coverage is 70.95%. Comparing base (26c6f1a) to head (d29df94).
Report is 1 commits behind head on unstable.

Files with missing lines	Patch %	Lines
src/cluster_legacy.c	14.66%	64 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #1572      +/-   ##
============================================
- Coverage     71.02%   70.95%   -0.08%     
============================================
  Files           121      121              
  Lines         65177    65225      +48     
============================================
- Hits          46295    46282      -13     
- Misses        18882    18943      +61     

Files with missing lines	Coverage Δ
src/module.c	9.59% <ø> (ø)
src/cluster_legacy.c	86.16% <14.66%> (-1.20%)	⬇️

... and 9 files with indirect coverage changes

[zuiderkwast]

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]

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.

[zuiderkwast]

Very nice! Some minor comments.

[zuiderkwast]

We don't check that the message receiver callback was called. Should we?

An idea is to let test.pingall be a blocking command that waits for the pong from each node and returns the number of replies, but maybe that's more complicated. You can decide.

[hpatro]

Yeah, we don't. We lack coverage for module message receiver callback. The stat counter ticks only if the payload is valid. So, we confirm that the target is able to process the packet i.e. if it's valid only.

I think it's beyond the scope of this PR to verify the callback. I will file an (good first) issue.

[zuiderkwast]

LGTM.

I added one suggestion but I'm not sure if it's objectively better or not so you can decide. :)

If you feel like adding some test coverage for the module message receiver callback API in another PR, that would be nice.

[zuiderkwast]

There are some committs from other PRs here now. Some rebased/merge mistake?

If you reset hard to before the last merge commit and then rebase again, the strange ones should disappear. Try that?

Anyway, seems good to merge.

[madolson]

Is it intentional the new test isn't getting run anywhere? Only the main test runs with other server, this one doesn't run anywhere.

[hpatro]

There are some committs from other PRs here now. Some rebased/merge mistake?

If you reset hard to before the last merge commit and then rebase again, the strange ones should disappear. Try that?

Anyway, seems good to merge.

Looks like a bad merge. Couldn't fix it via merge. Did a rebase and force pushed it.