jiajunwang commented on a change in pull request #1072:
URL: https://github.com/apache/helix/pull/1072#discussion_r447212931
##########
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:
What if ZK changes the default max buffer in some new versions? Are we
going to change this number accordingly? So my suggestion is that,
1. We fine the default value in Zookeeper code.
2. if 1 is not possible, then we can just use the current SIZE_LIMIT for the
default value.
##########
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:
The main concern is that you introduced some internal logic that is not
configurable or commented directly where the variables are defined. Who will be
able to tell the margin is derived by the default max buffer size and the
default size limit?
First, this logic is not reasonable, why the margin is based on the default
configs? Secondly, it introduces more chaos to our code.
##########
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:
Just add a MARGIN value please.
##########
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:
Let's just keep it strict for now.
##########
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;
+
+ // jute.maxbuffer is not directly used as size limit in ZkClient.
+ // Instead, leave some margin as ZNRecord.SIZE_LIMIT and use
(jute.maxbuffer - sizeMargin)
+ return Integer.getInteger(ZkSystemPropertyKeys.JUTE_MAXBUFFER,
DEFAULT_JUTE_MAXBUFFER)
Review comment:
So according to what I comment before, the logic here can be,
if (value of ZkSystemPropertyKeys.JUTE_MAXBUFFER exits) {
return value of ZkSystemPropertyKeys.JUTE_MAXBUFFER - MARGIN;
} else {
return SIZE_LIMIT;
}
----------------------------------------------------------------
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]
