You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Jun He (Code Review)" <ge...@cloudera.org> on 2016/12/25 07:04:40 UTC
[kudu-CR] [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.
Jun He has uploaded a new change for review.
http://gerrit.cloudera.org:8080/5578
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.
......................................................................
[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.
The new message framing will be
total_size: (32-bit big-endian integer)
the size of the rest of the message, not including this 4-byte header and also not including 4-byte checksum
header_checksum: (32-bit big-endian integer) CRC-32
the checksum of header (excluding varint-prefixed header size field)
- CRC-32 computation in C++ uses `boost/crc.hpp`.
- CRC-32 computation in Java is based on hadoop-common class, `org.apache.hadoop.util.PureJavaCrc32`
header: varint-prefixed header protobuf
- client->server messages use the RequestHeader protobuf
- server->client messages use the ResponseHeader protobuf
...
This is a non-backward compatible change.
Change-Id: Icf71ed4d4ac924445606b0d6fcf477d025243c74
---
M java/kudu-client/src/main/java/org/apache/kudu/client/CallResponse.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java
A java/kudu-client/src/main/java/org/apache/kudu/util/Crc32.java
A java/kudu-client/src/test/java/org/apache/kudu/client/TestRpcChecksum.java
M src/kudu/rpc/constants.h
M src/kudu/rpc/serialization.cc
6 files changed, 758 insertions(+), 12 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/78/5578/1
--
To view, visit http://gerrit.cloudera.org:8080/5578
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Icf71ed4d4ac924445606b0d6fcf477d025243c74
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jun He <ju...@gmail.com>
[kudu-CR] [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.
Posted by "Jun He (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5578
to look at the new patch set (#2).
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.
......................................................................
[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.
The new message framing will be
total_size: (32-bit big-endian integer)
the size of the rest of the message, not including this 4-byte header and also not including 4-byte checksum
header_checksum: (32-bit big-endian integer) CRC-32
the checksum of header (excluding varint-prefixed header size field)
- CRC-32 computation in C++ uses `boost/crc.hpp`.
- CRC-32 computation in Java is based on hadoop-common class, `org.apache.hadoop.util.PureJavaCrc32`
header: varint-prefixed header protobuf
- client->server messages use the RequestHeader protobuf
- server->client messages use the ResponseHeader protobuf
...
This is a non-backward compatible change.
Change-Id: Icf71ed4d4ac924445606b0d6fcf477d025243c74
---
M java/kudu-client/src/main/java/org/apache/kudu/client/CallResponse.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java
A java/kudu-client/src/main/java/org/apache/kudu/util/Crc32.java
A java/kudu-client/src/test/java/org/apache/kudu/client/TestRpcChecksum.java
M src/kudu/rpc/constants.h
M src/kudu/rpc/serialization.cc
6 files changed, 758 insertions(+), 12 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/78/5578/2
--
To view, visit http://gerrit.cloudera.org:8080/5578
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icf71ed4d4ac924445606b0d6fcf477d025243c74
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jun He <ju...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-90. Add a header checksum to our RPC protocol
Posted by "Jun He (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5578
to look at the new patch set (#4).
Change subject: KUDU-90. Add a header checksum to our RPC protocol
......................................................................
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.
The new message framing will be
total_size: (32-bit big-endian integer)
the size of the rest of the message, not including this 4-byte header
header_checksum: (32-bit big-endian integer) CRC-32C
the checksum of header (excluding varint-prefixed header size field)
- CRC-32C computation in C++ uses `kudu/util/crc.h`.
- CRC-32C computation in Java is based on hadoop-common class, `org.apache.hadoop.util.PureJavaCrc32C`
header: varint-prefixed header protobuf
- client->server messages use the RequestHeader protobuf
- server->client messages use the ResponseHeader protobuf
...
This is a non-backward compatible change.
Change-Id: Icf71ed4d4ac924445606b0d6fcf477d025243c74
---
M java/kudu-client/src/main/java/org/apache/kudu/client/CallResponse.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java
A java/kudu-client/src/main/java/org/apache/kudu/util/Crc32C.java
A java/kudu-client/src/test/java/org/apache/kudu/client/TestRpcChecksum.java
M src/kudu/rpc/constants.h
M src/kudu/rpc/serialization.cc
6 files changed, 763 insertions(+), 9 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/78/5578/4
--
To view, visit http://gerrit.cloudera.org:8080/5578
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icf71ed4d4ac924445606b0d6fcf477d025243c74
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jun He <ju...@gmail.com>
Gerrit-Reviewer: Jun He <ju...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [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.
Posted by "Jun He (Code Review)" <ge...@cloudera.org>.
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 <ju...@gmail.com>
Gerrit-Reviewer: Jun He <ju...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [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.
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 <ju...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [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.
Posted by "Jun He (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5578
to look at the new patch set (#3).
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.
......................................................................
[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.
The new message framing will be
total_size: (32-bit big-endian integer)
the size of the rest of the message, not including this 4-byte header
header_checksum: (32-bit big-endian integer) CRC-32C
the checksum of header (excluding varint-prefixed header size field)
- CRC-32C computation in C++ uses `kudu/util/crc.h`.
- CRC-32C computation in Java is based on hadoop-common class, `org.apache.hadoop.util.PureJavaCrc32C`
header: varint-prefixed header protobuf
- client->server messages use the RequestHeader protobuf
- server->client messages use the ResponseHeader protobuf
...
This is a non-backward compatible change.
Change-Id: Icf71ed4d4ac924445606b0d6fcf477d025243c74
---
M java/kudu-client/src/main/java/org/apache/kudu/client/CallResponse.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java
A java/kudu-client/src/main/java/org/apache/kudu/util/Crc32C.java
A java/kudu-client/src/test/java/org/apache/kudu/client/TestRpcChecksum.java
M src/kudu/rpc/constants.h
M src/kudu/rpc/serialization.cc
6 files changed, 763 insertions(+), 9 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/78/5578/3
--
To view, visit http://gerrit.cloudera.org:8080/5578
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icf71ed4d4ac924445606b0d6fcf477d025243c74
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jun He <ju...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins