Todd Lipcon 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: (6 comments) 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 checksum...) Line 8: Added 4-byte checksum into the RPC protocol. nit: please add a blank line before this one Line 25: This is a non-backward compatible change. I think it's too late to make a non-backward-compatible change. We should figure out a way to do this compatibly (eg by negotiating using feature flags during connection startup). We should also consider the checksumming carefully. A few thoughts here: - not sure why in the original JIRA we discussed only checksumming the header and not the body of the message. Maybe the checksum should trail the whole request/response and subsume the header and the body? - for security purposes we're planning on enabling TLS, which would add its own integrity checks which would subsume any checksumming we'd do. Even if the small overhead of encryption isn't desired, we can use a TLS cipher suite with just MAC and no encryption. Granted, SHA1 is ~8x slower than CRC32 so maybe it's still worth considering CRC? http://gerrit.cloudera.org:8080/#/c/5578/3/java/kudu-client/src/main/java/org/apache/kudu/client/CallResponse.java File java/kudu-client/src/main/java/org/apache/kudu/client/CallResponse.java: Line 72: "checksum mis-match error"); would this propagate up to the caller in a reasonable way? I don't think IllegalStateException is valid (more like an IOException) 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 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 checksums against a real server RPC to make sure that the error propagates back to the Kudu client user appropriately -- 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 <junhe...@gmail.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
