narendly commented on a change in pull request #1072:
URL: https://github.com/apache/helix/pull/1072#discussion_r437037198
##########
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:
Thanks for adding this test! One favor to ask - we are actually in the
process of moving this class to zookeeper-api
(https://github.com/apache/helix/pull/1070).
Do you think we could hold off on this change until that is committed first
to avoid merge conflicts?
##########
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:
I agree that this is the right thing to do. Thanks for catching this and
fixing this.
One concern I have is with the change of the exception type. I am not sure
if changing it to ZkMarshallingError is the right thing to do because
1. ZkMarshallingError is usually thrown in ZkSerializer implementations.
2. Changing Exception types may cause backward-compatibility issues.
What do you think?
##########
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:
I agree that this is the right thing to do. Thanks for catching this and
fixing this.
One concern I have is with the change of the exception type. I am not sure
if changing it to ZkMarshallingError is the right thing to do because
1. ZkMarshallingError is usually thrown in and reserved for use in
ZkSerializer implementations.
2. Changing Exception types may cause backward-compatibility issues.
What do you think?
##########
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:
To give you more context, ZkMarshallingError is something that was
inherited from 101tec.zkclient library and explicitly thrown for ZkSerializer
implementations. Since this is our ZkClient API-level check outside the
serializer, I think ZkClientException.
Also, we've decided to convert all HelixExceptions to ZkClientException, so
I think ZkClientException would be more appropriate.
##########
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:
To give you more context, ZkMarshallingError is something that was
inherited from 101tec.zkclient library and explicitly thrown for ZkSerializer
implementations. Since this is our ZkClient API-level check outside the
serializer, I think ZkClientException.
Also, we've decided to convert all HelixExceptions to ZkClientException, so
I think ZkClientException would be more appropriate for consistency.
##########
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:
To give you more context, ZkMarshallingError is something that was
inherited from 101tec.zkclient library and explicitly thrown for ZkSerializer
implementations. Since this is our ZkClient API-level check _outside_ the
serializer, I think ZkClientException.
Also, we've decided to convert all HelixExceptions to ZkClientException, so
I think ZkClientException would be more appropriate for consistency.
----------------------------------------------------------------
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]
