Adds Light-weight cluster bus header for pubsub message.

[roshkhatri]

Adds light-weight cluster bus header for pubsub message. Closes #557.

This also supports sending to and receiving non-light messages from older versions of the engine.
This light-weight header is defined below.

typedef struct {
    char sig[4];     /* Signature "RCmb" (Cluster message bus). */
    uint32_t totlen; /* Total length of this message */
    uint16_t ver;    /* Protocol version, currently set to CLUSTER_PROTO_VER. */
    uint16_t notused1;
    uint16_t type; /* Message type */
    uint16_t notused2;
    union clusterMsgData data;
} clusterMsgLight;

The PR also adds a special iterator for iterating over a list or a dict depending on the type of iterator initialized. This iterator is specially implemented here to go over the dict of all nodes present in the cluster for non-shaded pubsub and a list of nodes in a specific shard for sharded pubsub. This also reduces the code duplications.

[codecov]

Codecov Report

Attention: Patch coverage is 88.70968% with 14 lines in your changes missing coverage. Please review.

Project coverage is 70.34%. Comparing base (66d0f7d) to head (4431095).
Report is 19 commits behind head on unstable.

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable     #654      +/-   ##
============================================
+ Coverage     70.08%   70.34%   +0.26%     
============================================
  Files           112      112              
  Lines         60587    61400     +813     
============================================
+ Hits          42461    43192     +731     
- Misses        18126    18208      +82     

Files	Coverage Δ
src/cluster_legacy.c	85.10% <88.70%> (-0.61%)	⬇️

... and 26 files with indirect coverage changes

[zuiderkwast]

This is good progress!

I didn't comment on any details since it's still a draft.

[roshkhatri]

I have updated the draft allowing multiple payloads and new struct for publish message, would be adding tests and cleaning up the code.

[hpatro]

High level comments. Good progress 👍 .

[zuiderkwast]

This looks pretty good!

Did you try any tests yet? Maybe there will be no visible behavior change, so maybe no need for new tests, except for the number of bytes sent over the network. Do we have any stats for that we can verify in a test case?

[PingXie]

Are you suggesting we change this to be just a single payload and introduce two more light messages and yet another capability bit in the future for the multiple message feature? And each node needs to handle in total 6 pubsub messages (publish, spublish, light publish, light spublish, light multi-publish, light multi-spublish) with various subsets supported by different versions.

4 types (publish, spublish, light multi-publish, light multi-spublish) would suffice as we will hard-code the count value to 1 in 8.0, which will allow us to introduce the multi-message feature without a wire protocol change.

End users should not start using the mpublish style commands until all nodes in the cluster are updated.

Agreed. I don't think we can mandate the "N+1" upgrade. Valkey 7.2 users would have the same issue regardless, when upgrading to 8.2+.

Zooming out a bit more, I think this should be the general rule for all new features (sure, there will be exceptions, but hopefully rare). Supporting new features in a cluster of mixed versions adds a whole new dimension of complexity hence opening a can of worms for both development and testing IMO. There needs to be a very strong reason to go down that path.

[roshkhatri]

Tested sending and receiving publish message to and from older 7.2 version.

Not ready to merge anymore.

[PingXie]

Thanks @roshkhatri! I think we are very close. Some more nits for fit&finish. Let's aim at merging it in the next or two?

[PingXie]

LGTM. Thanks @roshkhatri!

[hpatro]

@PingXie Thanks for the review! @zuiderkwast / @madolson Do you folks want to take a look further or is it good to merge ?

[zuiderkwast]

Looks good. Just commented on some very minor things.

This implementation got a lot smaller, which is good, but I notice we'll not be able to add an mpublish cluster bus message without another protocol change, but we likely won't want that anyway.

[roshkhatri]

@PingXie @zuiderkwast @hpatro,Thank you so much for you support and review! Let's get it merged today/ASAP!🚀

[bbarani]

@PingXie @zuiderkwast @hpatro @madolson Do you have any other questions / comments? It would be great to get this merged for 8.0 release.

[zuiderkwast]

Now the commit message of the commit that got merged says the light pubsub message supports multiple payloads. That's unfortunate, since it's not true.

@roshkhatri please update the PR description when you have time, for future reference. The will probably be linked from the release notes.

[roshkhatri]

Updated the PR description. LMK if you think anything else needs to be added.