ZooKeeper
  1. ZooKeeper
  2. ZOOKEEPER-1383

Create update throughput quotas and add hard quota limits

    Details

    • Type: New Feature New Feature
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: server
    • Labels:
      None
    • Release Note:
      Adds support for throughput quotas (soft and hard) and hard node count and hard size quotas. Parses quota nodes from older versions of the server and preserves behavior of existing quotas (soft node count and soft size).
    • Tags:
      quotas

      Description

      Quotas exist for size (node count and size in bytes); it would be useful to track and support quotas on update throughput (bytes per second) as well. This can be tracked on both a node/subtree level for quota support as well as on the server level for monitoring.

      In addition, the existing quotas log a warning when they are exceeded but allow the transaction to proceed (soft quotas). It would also be useful to support a corresponding set of hard quota limits that fail the transaction.

      1. ZOOKEEPER-1383.patch
        104 kB
        Jay Shrauner
      2. ZOOKEEPER-1383.patch
        103 kB
        Jay Shrauner
      3. ZOOKEEPER-1383.patch
        70 kB
        Jay Shrauner
      4. ZOOKEEPER-1383.patch
        70 kB
        Jay Shrauner

        Activity

        Hide
        Jay Shrauner added a comment -

        Provided the idea sounds good, please review the attached change.

        Show
        Jay Shrauner added a comment - Provided the idea sounds good, please review the attached change.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12512700/ZOOKEEPER-1383.patch
        against trunk revision 1238176.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 15 new or modified tests.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        -1 findbugs. The patch appears to introduce 8 new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed core unit tests.

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/935//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/935//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/935//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12512700/ZOOKEEPER-1383.patch against trunk revision 1238176. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 15 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 8 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/935//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/935//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/935//console This message is automatically generated.
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12513711/ZOOKEEPER-1383.patch
        against trunk revision 1240959.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 15 new or modified tests.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed core unit tests.

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/952//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/952//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/952//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12513711/ZOOKEEPER-1383.patch against trunk revision 1240959. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 15 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/952//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/952//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/952//console This message is automatically generated.
        Hide
        Camille Fournier added a comment -

        This change is definitely going to break backwards compatibility of clients in a major way. I'm not sure that it can go into a 3.X release unless we can make it not break backwards compatibility.

        Show
        Camille Fournier added a comment - This change is definitely going to break backwards compatibility of clients in a major way. I'm not sure that it can go into a 3.X release unless we can make it not break backwards compatibility.
        Hide
        Camille Fournier added a comment -

        So, in short, I'm -1 on this until it stops breaking backwards compatibility. Might consider adding the update throughput quotas separately from hard quota limits.

        Show
        Camille Fournier added a comment - So, in short, I'm -1 on this until it stops breaking backwards compatibility. Might consider adding the update throughput quotas separately from hard quota limits.
        Hide
        Jay Shrauner added a comment -

        I looked at the old StatsNode deserialization code and it is possible to make this backwards compatible by exploiting some quirks in the old parsing code. The old stats nodes were serialized as

        "count=5,bytes=4"

        We can add new quota/stats types using the following format

        "count=5,bytes=4=;bytesPerSec=100;countHardLimit=10"

        The old parsing code will assert if there isn't exactly 1 ',', and it expects (but doesn't verify) that the first element is count and the second is bytes. So we put those two fields first and use semi-colons to separate all the new fields. The extra '=' after the bytes field is what tricks the old parsing into ignoring all the new fields.

        Show
        Jay Shrauner added a comment - I looked at the old StatsNode deserialization code and it is possible to make this backwards compatible by exploiting some quirks in the old parsing code. The old stats nodes were serialized as "count=5,bytes=4" We can add new quota/stats types using the following format "count=5,bytes=4=;bytesPerSec=100;countHardLimit=10" The old parsing code will assert if there isn't exactly 1 ',', and it expects (but doesn't verify) that the first element is count and the second is bytes. So we put those two fields first and use semi-colons to separate all the new fields. The extra '=' after the bytes field is what tricks the old parsing into ignoring all the new fields.
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12522060/ZOOKEEPER-1383.patch
        against trunk revision 1307644.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 15 new or modified tests.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed core unit tests.

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1029//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1029//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1029//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12522060/ZOOKEEPER-1383.patch against trunk revision 1307644. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 15 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1029//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1029//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1029//console This message is automatically generated.
        Hide
        Jay Shrauner added a comment -

        To clarify a bit: stats and quota nodes are serialized as strings, and the current code in StatsTrack expects them to serialize and parse identically with only 2 fields. This makes adding new fields problematic, and we need to add new fields to both stats and quota nodes for throughput quotas and we need to add new fields to quota nodes for hard limit quotas.

        Fortunately, the parsing code in the StatsTrack constructor leaves an opening to add new fields if we don't use commas and string them after an extra = sign after the 'bytes=4' portion of the string. It's definitely a bit hacky, but it works, and it seems the easiest way to allow us to add new fields. I've added new checks in the unit tests to verify that the current StatsTrack parser continues to parse correctly the new stats/quota nodes.

        I've also changed the parser so it can handle parsing unrecognized fields, which should make it easier to add new quota type in the future without having these backwards compatibility issues.

        Show
        Jay Shrauner added a comment - To clarify a bit: stats and quota nodes are serialized as strings, and the current code in StatsTrack expects them to serialize and parse identically with only 2 fields. This makes adding new fields problematic, and we need to add new fields to both stats and quota nodes for throughput quotas and we need to add new fields to quota nodes for hard limit quotas. Fortunately, the parsing code in the StatsTrack constructor leaves an opening to add new fields if we don't use commas and string them after an extra = sign after the 'bytes=4' portion of the string. It's definitely a bit hacky, but it works, and it seems the easiest way to allow us to add new fields. I've added new checks in the unit tests to verify that the current StatsTrack parser continues to parse correctly the new stats/quota nodes. I've also changed the parser so it can handle parsing unrecognized fields, which should make it easier to add new quota type in the future without having these backwards compatibility issues.
        Hide
        Jay Shrauner added a comment -

        Updated patch to remove extra copies of createQuota and delQuota that were left behind in ZooKeeperMain when it was refactored into separate classes in cli/*

        Show
        Jay Shrauner added a comment - Updated patch to remove extra copies of createQuota and delQuota that were left behind in ZooKeeperMain when it was refactored into separate classes in cli/*
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12522133/ZOOKEEPER-1383.patch
        against trunk revision 1307644.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 15 new or modified tests.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed core unit tests.

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1030//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1030//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1030//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12522133/ZOOKEEPER-1383.patch against trunk revision 1307644. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 15 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1030//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1030//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1030//console This message is automatically generated.
        Hide
        Camille Fournier added a comment -

        I'd like to get this into 3.5, can you put the patch in reviewboard for review?

        Thanks.

        Show
        Camille Fournier added a comment - I'd like to get this into 3.5, can you put the patch in reviewboard for review? Thanks.
        Hide
        Thawan Kooburat added a comment -

        Thanks for your interest in this patch. We recently found some issues with the current design and need some rework which probably miss 3.5 release.

        The current implementation added a logic in DataTree to decide when to reject a given request when quota is exceeded. This have some flaws, so we will have to change it so that Leader's PrepRequestProcessor makes this decision instead.

        Show
        Thawan Kooburat added a comment - Thanks for your interest in this patch. We recently found some issues with the current design and need some rework which probably miss 3.5 release. The current implementation added a logic in DataTree to decide when to reject a given request when quota is exceeded. This have some flaws, so we will have to change it so that Leader's PrepRequestProcessor makes this decision instead.
        Hide
        Thawan Kooburat added a comment -

        Need rework

        Show
        Thawan Kooburat added a comment - Need rework
        Hide
        Thawan Kooburat added a comment -

        RFC:

        Existing quota logic in ZooKeeper only used for keeping track of node count and byte usage per path. When “soft” limit is exceeded, the server log warning message. This is not sufficient for our operation requirement. Here is want we are planning to do. We already implemented majority of these functionalities except the hard limit.

        1. Resource metric – The system will be able to monitor the following resource usage and enforce hard limit.

        • node count
        • used bytes
        • write throughput (bytes/sec, transactions/sec)
        • read throughput (bytes/sec, transactions/sec) (monitoring only, no hard limit)

        2. Usage monitoring and soft-limit
        The server is going to export per-path usage statistic via four-letter command. Since this is easier for external monitoring system to get these numbers than reading from ZooKeeper directly. For example, the new command can report the following stats

        used_bytes.<path-A> 300
        used_bytes.<path-B> 500
        node_count.<path-A> 20
        node_count.<path-B> 40
        read_bytes.<path-A>.60 20 //Read byte/sec for the last 1 min
        read_bytes.<path-B>.60 20

        For read throughput and write throughput, all servers will report read throughput statistics but only the leader report write throughput statistic. Internally, we already used a high performance multi windows counters provided by Facebook’s jcommon (https://github.com/facebook/jcommon/blob/master/stats/src/main/java/com/facebook/stats/MultiWindowRate.java) However, I think the community may want a simpler counter to reduce the dependency requirement.

        Additional, I am going to add an option to disable soft-limit check since writing warning message to log file is not that useful and may affect performance (especially when replying txnlog and soft limit is exceeded).

        3. Hard limit
        PrepRequestProcessor on the leader have to decide when to reject a given request (instead of the current patch that rejects the request down in DataTree). However, PrepRequestProcessor will need to access more data from the DataTree in order to decide when to reject a request. There are 2 possible implementations here. First, usage tracking and limit checking is implemented in the DataTree, this is simpler given the amount of information that is available in DataTree itself. The problem is that this limit checking logic will not be accurate when there are in-flight requests. The other option is to move limit checking to PrepRequestProcessor and maintain in-flight statistic similar to ChangeRecord.

        I think the later option is much more complicate. Since it is ok to allow usage to exceed hard limit slightly, I might go with the first option.

        Let me know if you have any suggestion.

        Show
        Thawan Kooburat added a comment - RFC: Existing quota logic in ZooKeeper only used for keeping track of node count and byte usage per path. When “soft” limit is exceeded, the server log warning message. This is not sufficient for our operation requirement. Here is want we are planning to do. We already implemented majority of these functionalities except the hard limit. 1. Resource metric – The system will be able to monitor the following resource usage and enforce hard limit. node count used bytes write throughput (bytes/sec, transactions/sec) read throughput (bytes/sec, transactions/sec) (monitoring only, no hard limit) 2. Usage monitoring and soft-limit The server is going to export per-path usage statistic via four-letter command. Since this is easier for external monitoring system to get these numbers than reading from ZooKeeper directly. For example, the new command can report the following stats used_bytes.<path-A> 300 used_bytes.<path-B> 500 node_count.<path-A> 20 node_count.<path-B> 40 read_bytes.<path-A>.60 20 //Read byte/sec for the last 1 min read_bytes.<path-B>.60 20 For read throughput and write throughput, all servers will report read throughput statistics but only the leader report write throughput statistic. Internally, we already used a high performance multi windows counters provided by Facebook’s jcommon ( https://github.com/facebook/jcommon/blob/master/stats/src/main/java/com/facebook/stats/MultiWindowRate.java ) However, I think the community may want a simpler counter to reduce the dependency requirement. Additional, I am going to add an option to disable soft-limit check since writing warning message to log file is not that useful and may affect performance (especially when replying txnlog and soft limit is exceeded). 3. Hard limit PrepRequestProcessor on the leader have to decide when to reject a given request (instead of the current patch that rejects the request down in DataTree). However, PrepRequestProcessor will need to access more data from the DataTree in order to decide when to reject a request. There are 2 possible implementations here. First, usage tracking and limit checking is implemented in the DataTree, this is simpler given the amount of information that is available in DataTree itself. The problem is that this limit checking logic will not be accurate when there are in-flight requests. The other option is to move limit checking to PrepRequestProcessor and maintain in-flight statistic similar to ChangeRecord. I think the later option is much more complicate. Since it is ok to allow usage to exceed hard limit slightly, I might go with the first option. Let me know if you have any suggestion.

          People

          • Assignee:
            Thawan Kooburat
            Reporter:
            Jay Shrauner
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:

              Development