ZooKeeper
  1. ZooKeeper
  2. ZOOKEEPER-831

BookKeeper: Throttling improved for reads

    Details

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

      Description

      Reads and writes in BookKeeper are asymmetric: a write request writes one entry, whereas a read request may read multiple requests. The current implementation of throttling only counts the number of read requests instead of counting the number of entries being read. Consequently, a few read requests reading a large number of entries each will spawn a large number of read-entry requests.

      1. ZOOKEEPER-831.patch
        13 kB
        Flavio Junqueira
      2. ZOOKEEPER-831.patch
        11 kB
        Flavio Junqueira
      3. ZOOKEEPER-831.patch
        10 kB
        Flavio Junqueira
      4. ZOOKEEPER-831.patch
        6 kB
        Flavio Junqueira

        Issue Links

          Activity

          Hide
          Hudson added a comment -

          Integrated in ZooKeeper-trunk #940 (See https://hudson.apache.org/hudson/job/ZooKeeper-trunk/940/)
          ZOOKEEPER-831. BookKeeper: Throttling improved for reads

          Show
          Hudson added a comment - Integrated in ZooKeeper-trunk #940 (See https://hudson.apache.org/hudson/job/ZooKeeper-trunk/940/ ) ZOOKEEPER-831 . BookKeeper: Throttling improved for reads
          Hide
          Benjamin Reed added a comment -

          Committed revision 998200.

          thanx for the fix flavio and ivan for the reviews!

          Show
          Benjamin Reed added a comment - Committed revision 998200. thanx for the fix flavio and ivan for the reviews!
          Hide
          Ivan Kelly added a comment -

          +1

          Show
          Ivan Kelly added a comment - +1
          Hide
          Flavio Junqueira added a comment -

          Attaching a new patch. In this patch, asyncAddOp catches a RunTimeException and releases a permit if there is such an exception while calling submitOrdered. It also releases a permit if metadata.isClosed() evaluates to true.

          Show
          Flavio Junqueira added a comment - Attaching a new patch. In this patch, asyncAddOp catches a RunTimeException and releases a permit if there is such an exception while calling submitOrdered. It also releases a permit if metadata.isClosed() evaluates to true.
          Hide
          Ivan Kelly added a comment -

          submitOrdered can throw RejectedExecutionException (Im guessing rare to the order of "if this happens the machine will die soon") or NullPointerException (unlikely). However there's no harm putting a try { } catch (Exception e)

          { opCounterSem.release(); }

          around it. the handler will never run except if the job is never submitted.

          Also, you need a release inside the if (metadata.isClosed) . Otherwise I think it should be fine. As PendingAddOp should be able to handle it no matter what error occurs.

          Show
          Ivan Kelly added a comment - submitOrdered can throw RejectedExecutionException (Im guessing rare to the order of "if this happens the machine will die soon") or NullPointerException (unlikely). However there's no harm putting a try { } catch (Exception e) { opCounterSem.release(); } around it. the handler will never run except if the job is never submitted. Also, you need a release inside the if (metadata.isClosed) . Otherwise I think it should be fine. As PendingAddOp should be able to handle it no matter what error occurs.
          Hide
          Flavio Junqueira added a comment -

          I have implemented the modification Ivan suggested to avoid a new call in the API of LedgerHandler.

          The comment on the acquire call, however, is a little trickier. First, we can't acquire a permit in pendingAddOp.initiate() because the call to asyncAddEntry and the call to initiate are decoupled (a separate thread executes initiate). To have the acquire call being effective in throttling the client application, we need to put it either before or after submitOrdered in asyncAddEntry. Given that there is only one checked exception, which is thrown by the acquire call itself, it is my impression that it is ok to place the call before, and placing it before is the same order the we followed for read operations in pendingReadOp.initiate().

          What do you think, Ivan?

          Show
          Flavio Junqueira added a comment - I have implemented the modification Ivan suggested to avoid a new call in the API of LedgerHandler. The comment on the acquire call, however, is a little trickier. First, we can't acquire a permit in pendingAddOp.initiate() because the call to asyncAddEntry and the call to initiate are decoupled (a separate thread executes initiate). To have the acquire call being effective in throttling the client application, we need to put it either before or after submitOrdered in asyncAddEntry. Given that there is only one checked exception, which is thrown by the acquire call itself, it is my impression that it is ok to place the call before, and placing it before is the same order the we followed for read operations in pendingReadOp.initiate(). What do you think, Ivan?
          Hide
          Flavio Junqueira added a comment -

          Canceling patch until comments are addressed.

          Show
          Flavio Junqueira added a comment - Canceling patch until comments are addressed.
          Hide
          Ivan Kelly added a comment -

          You've added a public method to the API purely for testing. Is this necessary? You can access the private members using reflection, which would save you having to add anything to the API which could be difficult to remove in the future.

          @suppressWarning("unchecked")
          int getAvailablePermits(LedgerHandle lh)

          { Field field = LedgerHandle.class.getDeclaredField("opCounterSem"); field.setAccessible(true); return ((Semaphore)field.get(lh)).getAvailablePermits(); }
          Show
          Ivan Kelly added a comment - You've added a public method to the API purely for testing. Is this necessary? You can access the private members using reflection, which would save you having to add anything to the API which could be difficult to remove in the future. @suppressWarning("unchecked") int getAvailablePermits(LedgerHandle lh) { Field field = LedgerHandle.class.getDeclaredField("opCounterSem"); field.setAccessible(true); return ((Semaphore)field.get(lh)).getAvailablePermits(); }
          Hide
          Ivan Kelly added a comment -

          asyncAddEntry - why did you move the semaphore acquire in this? If an exception occurs during the adding of the operation, is the completion (and therefore the release) guaranteed?

          Otherwise the patch looks good.

          Show
          Ivan Kelly added a comment - asyncAddEntry - why did you move the semaphore acquire in this? If an exception occurs during the adding of the operation, is the completion (and therefore the release) guaranteed? Otherwise the patch looks good.
          Hide
          Flavio Junqueira added a comment -

          Submitting new patch.

          Show
          Flavio Junqueira added a comment - Submitting new patch.
          Hide
          Flavio Junqueira added a comment -

          Need to fix log messages.

          Show
          Flavio Junqueira added a comment - Need to fix log messages.
          Hide
          Hadoop QA added a comment -

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

          +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/107/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h7.grid.sp2.yahoo.net/107/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h7.grid.sp2.yahoo.net/107/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/12451268/ZOOKEEPER-831.patch against trunk revision 980576. +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/107/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h7.grid.sp2.yahoo.net/107/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h7.grid.sp2.yahoo.net/107/console This message is automatically generated.
          Hide
          Flavio Junqueira added a comment -

          This patch fixes the issue described and modifies a test to verify that it is working.

          Show
          Flavio Junqueira added a comment - This patch fixes the issue described and modifies a test to verify that it is working.

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development