Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 3.3.0
    • Fix Version/s: 3.4.0
    • Component/s: contrib-bookkeeper
    • Labels:
      None

      Description

      Add throttling to client to control the rate of operations to bookies.

      1. ZOOKEEPER-719.patch
        15 kB
        Flavio Junqueira
      2. ZOOKEEPER-719.patch
        11 kB
        Flavio Junqueira
      3. ZOOKEEPER-719.patch
        12 kB
        Flavio Junqueira
      4. ZOOKEEPER-719.patch
        5 kB
        Flavio Junqueira

        Activity

        Flavio Junqueira created issue -
        Patrick Hunt made changes -
        Field Original Value New Value
        Fix Version/s 3.4.0 [ 12314469 ]
        Affects Version/s 3.3.0 [ 12313976 ]
        Hide
        Patrick Hunt added a comment -

        Is this really a bug or improvement? If not a bug it should go to 3.4.0 and not 3.3.1. Marking for 3.3.1/3.4.0 for now.

        Show
        Patrick Hunt added a comment - Is this really a bug or improvement? If not a bug it should go to 3.4.0 and not 3.3.1. Marking for 3.3.1/3.4.0 for now.
        Hide
        Flavio Junqueira added a comment -

        I'm not sure how to classify this one. In my test cases, the only problem it causes me so far is that it makes it more difficult to measure throughput because some bookies can be arbitrarily ahead of others in an ensemble while writing. One could see it as a bug too, I suppose, since throttling is desirable to keep the flow of requests under control.

        It is fine to have it marked for 3.4.0 for me.

        Show
        Flavio Junqueira added a comment - I'm not sure how to classify this one. In my test cases, the only problem it causes me so far is that it makes it more difficult to measure throughput because some bookies can be arbitrarily ahead of others in an ensemble while writing. One could see it as a bug too, I suppose, since throttling is desirable to keep the flow of requests under control. It is fine to have it marked for 3.4.0 for me.
        Patrick Hunt made changes -
        Fix Version/s 3.3.1 [ 12314846 ]
        Hide
        Flavio Junqueira added a comment -

        I added a semaphore object to count the number of writes and reads pending to LedgerHandle. This solution does not cause a deadlock in the case of bookie crashes, which happens if we change the pendingAddOps ArrayDeque to a blocking data structure.

        Show
        Flavio Junqueira added a comment - I added a semaphore object to count the number of writes and reads pending to LedgerHandle. This solution does not cause a deadlock in the case of bookie crashes, which happens if we change the pendingAddOps ArrayDeque to a blocking data structure.
        Flavio Junqueira made changes -
        Attachment ZOOKEEPER-719.patch [ 12440549 ]
        Flavio Junqueira made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Assignee Flavio Paiva Junqueira [ fpj ]
        Hide
        Hadoop QA added a comment -

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

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

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no tests are needed for this patch.

        +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 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: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/41/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/41/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/41/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/12440549/ZOOKEEPER-719.patch against trunk revision 929564. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no tests are needed for this patch. +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 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: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/41/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/41/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/41/console This message is automatically generated.
        Hide
        Flavio Junqueira added a comment -

        I'm not happy with this patch for two reasons:

        1. In this current form, it is not possible to set the maximum number of outstanding operations;
        2. Because of the use of a semaphore, I had to catch InterruptedException in a few places. The most critical ones for me are the ones in the procedure to recover a ledger. I believe in the way I have currently implemented, it could cause problems if an instance of InterruptedException is raised when recovering a ledger, since the procedure could complete successfully. I need to investigate this further and either determine that it isn't a problem or find a better way.

        Any suggestion would be appreciated.

        Show
        Flavio Junqueira added a comment - I'm not happy with this patch for two reasons: In this current form, it is not possible to set the maximum number of outstanding operations; Because of the use of a semaphore, I had to catch InterruptedException in a few places. The most critical ones for me are the ones in the procedure to recover a ledger. I believe in the way I have currently implemented, it could cause problems if an instance of InterruptedException is raised when recovering a ledger, since the procedure could complete successfully. I need to investigate this further and either determine that it isn't a problem or find a better way. Any suggestion would be appreciated.
        Flavio Junqueira made changes -
        Status Patch Available [ 10002 ] Open [ 1 ]
        Hide
        Patrick Hunt added a comment -

        3. tests missing

        Show
        Patrick Hunt added a comment - 3. tests missing
        Hide
        Flavio Junqueira added a comment -

        I have added a call to set the throttling threshold in LedgerHandle and a test to set the threshold value while reading and writing. I have also added calls to interrupt the current thread in the catch block of InterruptedException in LedgerRecoveryOp.

        Show
        Flavio Junqueira added a comment - I have added a call to set the throttling threshold in LedgerHandle and a test to set the threshold value while reading and writing. I have also added calls to interrupt the current thread in the catch block of InterruptedException in LedgerRecoveryOp.
        Flavio Junqueira made changes -
        Attachment ZOOKEEPER-719.patch [ 12445236 ]
        Flavio Junqueira made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Hide
        Hadoop QA added a comment -

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

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

        +1 tests included. The patch appears to include 3 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 warnings.

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

        -1 core tests. The patch failed core unit tests.

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

        Test results: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/103/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/103/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/103/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/12445236/ZOOKEEPER-719.patch against trunk revision 947063. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 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 warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/103/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/103/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/103/console This message is automatically generated.
        Hide
        Flavio Junqueira added a comment -

        The core test failure is unrelated to this patch. This patch does not affect any ZK core code.

        Show
        Flavio Junqueira added a comment - The core test failure is unrelated to this patch. This patch does not affect any ZK core code.
        Hide
        Benjamin Reed added a comment -

        there are a couple of problems:

        1) you seem to have a stray opCounterSem in PerClientBookieClient. you define it, but you never use it.

        2) i think it might be better to use a system property to set the throttling rather than allow it to be dynamically changed. it simplifies the code. setThrottle is especially problematic since you are catching InterruptedException and it isn't thread safe.

        Show
        Benjamin Reed added a comment - there are a couple of problems: 1) you seem to have a stray opCounterSem in PerClientBookieClient. you define it, but you never use it. 2) i think it might be better to use a system property to set the throttling rather than allow it to be dynamically changed. it simplifies the code. setThrottle is especially problematic since you are catching InterruptedException and it isn't thread safe.
        Hide
        Flavio Junqueira added a comment -

        Thanks for the comments, Ben. I see three options to set the throttling threshold:

        1. Use a configuration file to set the parameter value. Unfortunately, there are a couple of issues to this option. First, we currently don't have a config file on the client side, and I'm not sure if one variable would justify setting it up. Second, I'm not convinced that it is a good idea to have config files on the client side. It is nice to instantiate a BookKeeper object without having to bother about config files (on the client side);
        2. Add an input variable to BookKeeper constructors or to createLedger/openLedger calls. This one sounds like a bad option to me because I don't think it will be used often and it is not related to the corresponding operations;
        3. Use set/get calls to change the threshold value. This is what I used with this patch.

        A fourth option is to have an environment variable, but this is similar to having a config file, so I'm not sure there is any great advantage.

        Any thoughts?

        Show
        Flavio Junqueira added a comment - Thanks for the comments, Ben. I see three options to set the throttling threshold: Use a configuration file to set the parameter value. Unfortunately, there are a couple of issues to this option. First, we currently don't have a config file on the client side, and I'm not sure if one variable would justify setting it up. Second, I'm not convinced that it is a good idea to have config files on the client side. It is nice to instantiate a BookKeeper object without having to bother about config files (on the client side); Add an input variable to BookKeeper constructors or to createLedger/openLedger calls. This one sounds like a bad option to me because I don't think it will be used often and it is not related to the corresponding operations; Use set/get calls to change the threshold value. This is what I used with this patch. A fourth option is to have an environment variable, but this is similar to having a config file, so I'm not sure there is any great advantage. Any thoughts?
        Hide
        Flavio Junqueira added a comment -

        Canceling patch until we decide how to control throttling on the client.

        Show
        Flavio Junqueira added a comment - Canceling patch until we decide how to control throttling on the client.
        Flavio Junqueira made changes -
        Status Patch Available [ 10002 ] Open [ 1 ]
        Hide
        Benjamin Reed added a comment -

        i think using a system property is still the easiest, but i'm fine with the set/get if you want to do it. you just need to make it thread safe.

        Show
        Benjamin Reed added a comment - i think using a system property is still the easiest, but i'm fine with the set/get if you want to do it. you just need to make it thread safe.
        Hide
        Flavio Junqueira added a comment -

        Submitting a new patch that uses a system property.

        In LedgerHandle, I'm currently throwing NumberFormatException instead of catching it locally. My rationale is that if a user is trying to set the throttling threshold, then we should not proceed when the input value is not correct. To catch and handle a NFE exception that may arise in both LedgerCreateOp and LedgerOpenOp, I created a new BKException called BKIncorrectParameterException.

        Show
        Flavio Junqueira added a comment - Submitting a new patch that uses a system property. In LedgerHandle, I'm currently throwing NumberFormatException instead of catching it locally. My rationale is that if a user is trying to set the throttling threshold, then we should not proceed when the input value is not correct. To catch and handle a NFE exception that may arise in both LedgerCreateOp and LedgerOpenOp, I created a new BKException called BKIncorrectParameterException.
        Flavio Junqueira made changes -
        Attachment ZOOKEEPER-719.patch [ 12448407 ]
        Flavio Junqueira made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Hide
        Hadoop QA added a comment -

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

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

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no tests are needed for this patch.

        +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 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: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h7.grid.sp2.yahoo.net/91/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h7.grid.sp2.yahoo.net/91/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h7.grid.sp2.yahoo.net/91/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/12448407/ZOOKEEPER-719.patch against trunk revision 958096. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no tests are needed for this patch. +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 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: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h7.grid.sp2.yahoo.net/91/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h7.grid.sp2.yahoo.net/91/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h7.grid.sp2.yahoo.net/91/console This message is automatically generated.
        Hide
        Flavio Junqueira added a comment -

        My bad, forgot to include the test file in the patch... Resubmitting in a second.

        Show
        Flavio Junqueira added a comment - My bad, forgot to include the test file in the patch... Resubmitting in a second.
        Flavio Junqueira made changes -
        Status Patch Available [ 10002 ] Open [ 1 ]
        Flavio Junqueira made changes -
        Attachment ZOOKEEPER-719.patch [ 12448478 ]
        Flavio Junqueira made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Hide
        Hadoop QA added a comment -

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

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

        +1 tests included. The patch appears to include 3 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 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: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h7.grid.sp2.yahoo.net/92/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h7.grid.sp2.yahoo.net/92/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h7.grid.sp2.yahoo.net/92/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/12448478/ZOOKEEPER-719.patch against trunk revision 958096. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 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 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: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h7.grid.sp2.yahoo.net/92/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h7.grid.sp2.yahoo.net/92/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h7.grid.sp2.yahoo.net/92/console This message is automatically generated.
        Hide
        Benjamin Reed added a comment -

        +1 looks good

        Show
        Benjamin Reed added a comment - +1 looks good
        Hide
        Benjamin Reed added a comment -

        Committed revision 962693.

        Show
        Benjamin Reed added a comment - Committed revision 962693.
        Benjamin Reed made changes -
        Status Patch Available [ 10002 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Hide
        Hudson added a comment -

        Integrated in ZooKeeper-trunk #881 (See http://hudson.zones.apache.org/hudson/job/ZooKeeper-trunk/881/)

        Show
        Hudson added a comment - Integrated in ZooKeeper-trunk #881 (See http://hudson.zones.apache.org/hudson/job/ZooKeeper-trunk/881/ )
        Mahadev konar made changes -
        Status Resolved [ 5 ] Closed [ 6 ]

          People

          • Assignee:
            Flavio Junqueira
            Reporter:
            Flavio Junqueira
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development