[WIP] Skip the checksum for diskless replication

[Shivshankar-Reddy]

fixes: #1129
Below is the background information from #1129 by @madolson

When we are not writing to disk (either on the primary or replica) there isn't a lot of sense in doing CRC64 checksumming during fullsync. We do checking summing at the TCP layer already. At AWS we've seen it can take up to 15% overhead for not much of a benefit. We should look into a mechanism so that a replica can indicate that it will load something directly into memory, since today we only indicate we support the EOF marker. The replica might still choose to save the RDB file to disk unless repl-diskless-load is enabled. With this mechanism in place, we should be able to skip the checksum for replication, while still using it for RDB.

Here is my understanding and proposal as a fix for the issue. Please correct me if my understanding is wrong or missing something!

• Added 'replication-diskless-load' to sendCommand replica details in syncWithPrimary and FullSyncWithPrimary.
• Added rdbFlag to skip checksum based on 'replica-diskless-load' configuaration from the replica.

[hpatro]

Do we avoid any kind of memory corruption by performing a checksum match? If CRC64 is expensive, we could look at other alternatives.

[ranshid]

@Shivshankar-Reddy Sorry had some urgent issues and holidays so was unable to look at it yet.
Overall I like this added ability!

General question I have: I noticed that you only disabled CRC calculation on the primary side? Basically both primary and replica are performing the CRC calculation. When the replica is completely aware that the data is sent and loaded diskless it can just skip CRC check even when the primary did calculate and report it (for example a replica syncing with an older version primary). the replica can probably know that the Primary is streaming via socket by the fact that it send the EOF marker.

[ranshid]

we should probably keep only one rdbSaveRio :)

[Shivshankar-Reddy]

Thank you for noticing this. I am aware of this and removed in my local branch, this has had happened because I had merge conflict in the branch so created this by copying the code from original branch. it was missed during the copy paste

[ranshid]

I think at this point the skip CRC is more "capability" than "requirement" on the replica side. So I would suggest adding a replconf capa indication instead.

[Shivshankar-Reddy]

Initially my plan also was to add in capabilities, However the capabilies were not passed many function as requirements being passed. Since requirements are so flexible in the available functions I added in the requirements. But I agree with you this should be part of the capa. PR is still in-progress I will try to make this in Capabilities instead of requirements. Thank you!

[Shivshankar-Reddy]

Thank you for the review and the input!

General question I have: I noticed that you only disabled CRC calculation on the primary side? Basically both primary and replica are performing the CRC calculation. When the replica is completely aware that the data is sent and loaded diskless it can just skip CRC check even when the primary did calculate and report it (for example a replica syncing with an older version primary). the replica can probably know that the Primary is streaming via socket by the fact that it send the EOF marker.

In my initial testing, found that replica was able to deteremine CRC is enabled/disabled so I thought it would be enough to disable at primary end. and I think you have the valid point to consider replica side aswell. I will look into this and will update the diff soon, thanks again!

[ranshid]

@Shivshankar-Reddy thank you so much for taking the time to handle this. I think that since it was in conflict with #1479 (which is almost completed) we will close this one.