[GitHub] [helix] pkuwm commented on a change in pull request #1072: Make data size limit in ZkClient to use znrecord size config

[GitBox <gi...@apache.org>]

pkuwm commented on a change in pull request #1072:
URL: https://github.com/apache/helix/pull/1072#discussion_r449375143



##########
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:
       Same reply above: @jiajunwang Rethinking about this, I don't think we need this margin. Eg. if we set jute.maxbuffer=1024KB, with the margin=24KB, when we write a znode 1023KB, it fails because 1023KB > 1000KB. This is confusing. So I think we should not bring in the 24KB into custom jute.maxbuffer. Same as ZNRecord serializer write size limit, if we set it, we use the number directly without subtracting the margin.
   So I think it is clear and transparent to use the configured value without subtracting the margin. What do you think?
   
   Let's use the thread above to discuss.

##########
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:
       @jiajunwang Rethinking about this, I don't think we need this margin. Eg. if we set jute.maxbuffer=1024KB, with the margin=24KB, when we write a znode 1023KB, it fails because 1023KB > 1000KB. This is confusing. So I think we should not bring in the 24KB into custom jute.maxbuffer. Same as ZNRecord serializer write size limit, if we set it, we use the number directly without subtracting the margin.
   So I think it is clear and transparent to use the configured value without subtracting the margin. What do you think?




----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org