[GitHub] [zookeeper] hanm commented on a change in pull request #934: ZOOKEEPER-3301:Enforce the quota limit

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

hanm commented on a change in pull request #934:
URL: https://github.com/apache/zookeeper/pull/934#discussion_r503653071



##########
File path: zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md
##########
@@ -856,6 +856,10 @@ property, when available, is noted below.
     commit log is triggered.
     Does not affect the limit defined by *flushDelay*.
     Default is 1000.
+* *enforeQuota* :
+    (Java system property: **zookeeper.enforeQuota**)
+    enfore the quota check. Enable this option to use the [quota feature](http://zookeeper.apache.org/doc/current/zookeeperQuotas.html).
+    the default value:true.

Review comment:
       +1 on disabling the feature by default 

##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/cli/SetQuotaCommand.java
##########
@@ -158,39 +220,79 @@ public static boolean createQuota(
         for (int i = 1; i < splits.length; i++) {
             sb.append("/").append(splits[i]);
             quotaPath = sb.toString();
-            try {
-                zk.create(quotaPath, null, ZooDefs.Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT);
-            } catch (KeeperException.NodeExistsException ne) {
-                //do nothing
+            if (zk.exists(quotaPath, false) == null) {
+                try {
+                    zk.create(quotaPath, null, ZooDefs.Ids.OPEN_ACL_UNSAFE,
+                            CreateMode.PERSISTENT);
+                } catch (KeeperException.NodeExistsException ne) {
+                    //do nothing
+                }
             }
         }
         String statPath = quotaPath + "/" + Quotas.statNode;
         quotaPath = quotaPath + "/" + Quotas.limitNode;
-        StatsTrack strack = new StatsTrack(null);
-        strack.setBytes(bytes);
-        strack.setCount(numNodes);
-        try {
-            zk.create(quotaPath, strack.toString().getBytes(), ZooDefs.Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT);
-            StatsTrack stats = new StatsTrack(null);
-            stats.setBytes(0L);
+        byte[] data;
+
+        if (zk.exists(quotaPath, false) == null) {
+            zk.create(quotaPath, quota.toString().getBytes(),
+                    ZooDefs.Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT);
+
+            StatsTrack stats = new StatsTrack();
             stats.setCount(0);
-            zk.create(statPath, stats.toString().getBytes(), ZooDefs.Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT);
-        } catch (KeeperException.NodeExistsException ne) {
-            byte[] data = zk.getData(quotaPath, false, new Stat());
-            StatsTrack strackC = new StatsTrack(new String(data));
-            if (bytes != -1L) {
-                strackC.setBytes(bytes);
+            stats.setBytes(0L);
+
+            zk.create(statPath, stats.toString().getBytes(),
+                    ZooDefs.Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT);
+
+            data = zk.getData(quotaPath, false, new Stat());
+            StatsTrack quotaStrack = new StatsTrack(data);
+
+            data = zk.getData(statPath, false, new Stat());
+            StatsTrack statStrack = new StatsTrack(data);
+            if ((quotaStrack.getCount() > -1 && quotaStrack.getCount() < statStrack.getCount()) || (quotaStrack.getCountHardLimit() > -1

Review comment:
       There are quite a few checks in this code block that can be extracted into some sort of `checkQuota` abstraction




----------------------------------------------------------------
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