pkuwm commented on a change in pull request #1072:
URL: https://github.com/apache/helix/pull/1072#discussion_r443111837
##########
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java
##########
@@ -2216,4 +2236,24 @@ private void validateCurrentThread() {
throw new IllegalArgumentException("Must not be done in the zookeeper
event thread.");
}
}
+
+ private void validateWriteSizeLimitConfig() {
+ int serializerSize = ZNRecordUtil.getSerializerWriteSizeLimit();
+ int zkClientSize = getWriteSizeLimit();
+ // ZNRecord serializer write size limit should not be set greater than
size limit in ZkClient
+ if (serializerSize > zkClientSize) {
+ throw new IllegalStateException("ZNRecord serializer write size limit: "
+ serializerSize
+ + " is greater than size limit in ZkClient: " + zkClientSize);
+ }
+ }
+
+ private int getWriteSizeLimit() {
+ // The size margin left in default SIZE_LIMIT
+ int sizeMargin = DEFAULT_JUTE_MAXBUFFER - ZNRecord.SIZE_LIMIT;
Review comment:
ZNRecordSerializer is also using the SIZE_LIMIT to determine the actual
size limit. Maybe we still need it. Otherwise, we still have `sizeLimit =
JUTE_MAXBUFFER - MARGIN`. I understand this calculation is awkward, but just it
is just a local variable. Or we could add one more constant for margin so we
don't have to calculate it again here.
##########
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java
##########
@@ -73,6 +84,10 @@
private static Logger LOG = LoggerFactory.getLogger(ZkClient.class);
private static long MAX_RECONNECT_INTERVAL_MS = 30000; // 30 seconds
+ // Default value for system property jute.maxbuffer
+ // It specifies the maximum size of the data that can be stored in a znode.
+ private static final int DEFAULT_JUTE_MAXBUFFER = 0xfffff;
Review comment:
Answer is no. That one is different: it's for the response packet size
and it's 4 MB. We are looking at server side jute.maxbuffer that is by default
1 MB in `BinaryInputArchive`. It does no harm to define our own private
constant and the default value 1 MB is known.
##########
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java
##########
@@ -2216,4 +2236,24 @@ private void validateCurrentThread() {
throw new IllegalArgumentException("Must not be done in the zookeeper
event thread.");
}
}
+
+ private void validateWriteSizeLimitConfig() {
+ int serializerSize = ZNRecordUtil.getSerializerWriteSizeLimit();
+ int zkClientSize = getWriteSizeLimit();
+ // ZNRecord serializer write size limit should not be set greater than
size limit in ZkClient
+ if (serializerSize > zkClientSize) {
+ throw new IllegalStateException("ZNRecord serializer write size limit: "
+ serializerSize
Review comment:
In this case, we could add one more check for compression on/off. If
compression is turned on, we allow serializerSize > zkClientSize. Agreed?
----------------------------------------------------------------
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]
