pkuwm commented on a change in pull request #1072:
URL: https://github.com/apache/helix/pull/1072#discussion_r437070401
##########
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java
##########
@@ -1839,12 +1848,15 @@ protected void doRetry() {
});
}
- private void checkDataSizeLimit(byte[] data) {
- if (data != null && data.length > ZNRecord.SIZE_LIMIT) {
- LOG.error(
- "Data size larger than 1M, will not write to zk. Data (first 1k): "
+ new String(data)
- .substring(0, 1024));
- throw new ZkClientException("Data size larger than 1M");
+ private void checkDataSizeLimit(String path, byte[] data) {
+ if (data == null) {
+ return;
+ }
+
+ int sizeLimit = ZNRecordUtil.getSerializerWriteSizeLimit();
+ if (data.length > sizeLimit) {
+ throw new ZkMarshallingError(
Review comment:
These questions are valid and natural ones which I also thought about.
1. The reason I change it is I would like to make it consistent with
serializer. In helix zkclient, the data size check is right after serializing.
`ZkMarshallingError` could also indicate the serialized data still exceeds the
limit. So I feel `ZkMarshallingError` is more accurate than `ZkClientException`.
2. Actually before version 1.0.0, the exception type is `HelixException`. So
changing to `ZkClientException` already breaks backward-compatibility if it
breaks. Considering recent 1.0.0 zookeeper-api module users(helix-core 1.0.0 is
deprecated), if a user explicitly uses this raw zkclient and catches this
runtime exception, that may break. But I am not sure if that really exists in
1.0.0 usage, considering the APIs don't explicitly signature
`ZkClientException`.
Though having these thoughts, I don't have a strong preference: we could
keep the original exception type. If we would like to change the type, may be
it is a good idea to do it in 1.0.1 to avoid future backward compatibility
issue.
##########
File path:
helix-core/src/test/java/org/apache/helix/manager/zk/TestRawZkClient.java
##########
@@ -819,4 +820,45 @@ public void testAsyncWriteOperations() {
zkClient.delete("/tmp/asyncOversize");
}
}
+
+ @Test(expectedExceptions = ZkMarshallingError.class,
Review comment:
That's fine. Once the PR is committed, I will resolve the conflicts.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
