Jun He has posted comments on this change. Change subject: [KUDU-90] Add a header checksum to our RPC protocol Added 4-byte checksum into the RPC protocol. Checksum is inserted immediately after total_size. ......................................................................
Patch Set 3: (5 comments) As the cost of computing CRC-32C is quite low, we may still consider to have it if TLS is only optional. http://gerrit.cloudera.org:8080/#/c/5578/3//COMMIT_MSG Commit Message: Line 7: [KUDU-90] Add a header checksum to our RPC protocol > nit: follow the format used by other commit messages (KUDU-90. Add a header Done Line 8: Added 4-byte checksum into the RPC protocol. > nit: please add a blank line before this one Done Line 25: This is a non-backward compatible change. > I think it's too late to make a non-backward-compatible change. We should f For the checksum, I think it makes sense to have the checksum for the whole message. For the compatibility issue, a straightforward way is to add an optional field in pb header schema to store CRC. But this will complicate the checksum computation and also will add 4-byte data in header object. Feature flags sounds like another good way to solve the compatibility issue. If TLS will be always enabled, we don't need additional CRC. But if it is optional, I think CRC is still valuable if we only care network error instead of integrity. So if TLS is enabled, we can skip CRC, otherwise, we add CRC for the new protocol. I think enabling TLS will face the similar compatibility problem. Please let me know your comments. Thanks. http://gerrit.cloudera.org:8080/#/c/5578/3/java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java File java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java: Line 74: public static final int INT_SIZE_IN_BYTES = 4; > Integer.SIZE / 8 Done http://gerrit.cloudera.org:8080/#/c/5578/3/java/kudu-client/src/test/java/org/apache/kudu/client/TestRpcChecksum.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestRpcChecksum.java: Line 26: public class TestRpcChecksum { > this is nice as a unit test but would be good to actually inject invalid ch This is a great idea. Will add it once we decide how to proceed. -- To view, visit http://gerrit.cloudera.org:8080/5578 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Icf71ed4d4ac924445606b0d6fcf477d025243c74 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jun He <[email protected]> Gerrit-Reviewer: Jun He <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
