Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.3.0
    • Component/s: contrib-bookkeeper
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    • Release Note:
      bookkeeper code rewrite with optimization on disk write scheduling and scheduling of ledger writes on the client side.

      Description

      Error handling is far from ideal currently in the BookKeeper client.

      1. ZOOKEEPER-507.patch
        516 kB
        Benjamin Reed
      2. ZOOKEEPER-507.patch
        516 kB
        Utkarsh Srivastava
      3. ZOOKEEPER-507.patch
        511 kB
        Flavio Junqueira
      4. BookieFailureTest-log-507.rtf
        81 kB
        Flavio Junqueira
      5. ZOOKEEPER-507.patch
        510 kB
        Flavio Junqueira
      6. ZOOKEEPER-507.patch
        520 kB
        Utkarsh Srivastava

        Issue Links

          Activity

          Hide
          Utkarsh Srivastava added a comment -

          A major rewrite of the bookkeeper client library

          Show
          Utkarsh Srivastava added a comment - A major rewrite of the bookkeeper client library
          Hide
          Utkarsh Srivastava added a comment -

          We've been using this modified version of bookkeeper for some time with good results in terms of both stability and performance.

          This patch incorporates a couple of major changes, so its better reviewed as a whole, rather than as a diff.

          The two major changes are:

          i) Rewrite of the client library to deal with error cases, and generally iron out a lot of bugs.
          ii) Enhancing the bookie server to deal with a large number of open ledgers by multiplexing them into a common number of files.

          An added dependency is netty, a jboss project for network i/o. Netty is under an Apache 2.0 license, and the jar can be grabbed from
          http://repository.jboss.com/maven2/org/jboss/netty/netty/3.1.2.GA/netty-3.1.2.GA.jar

          Show
          Utkarsh Srivastava added a comment - We've been using this modified version of bookkeeper for some time with good results in terms of both stability and performance. This patch incorporates a couple of major changes, so its better reviewed as a whole, rather than as a diff. The two major changes are: i) Rewrite of the client library to deal with error cases, and generally iron out a lot of bugs. ii) Enhancing the bookie server to deal with a large number of open ledgers by multiplexing them into a common number of files. An added dependency is netty, a jboss project for network i/o. Netty is under an Apache 2.0 license, and the jar can be grabbed from http://repository.jboss.com/maven2/org/jboss/netty/netty/3.1.2.GA/netty-3.1.2.GA.jar
          Hide
          Flavio Junqueira added a comment -

          Sorry for the delay, I'll be reviewing this shortly.

          Show
          Flavio Junqueira added a comment - Sorry for the delay, I'll be reviewing this shortly.
          Hide
          Flavio Junqueira added a comment -

          One small comment at this point is that we have no lib folder under bookkeeper, and Pat tells me that putting under PROJECT_ROOT/src/java/lib is not ok. So, it doesn't compile without modifying build.xml.

          I see two options:

          1. We create a bookkeeper/src/java/lib folder and add the netty jar. In this case, we edit build.xml to set its classpath appropriately;
          2. We make it use ivy and fetch from a repository.

          Another point is that for the new bookie classes, I haven't seen javadocs or comments at all. I think it would be great to have some in there.

          Show
          Flavio Junqueira added a comment - One small comment at this point is that we have no lib folder under bookkeeper, and Pat tells me that putting under PROJECT_ROOT/src/java/lib is not ok. So, it doesn't compile without modifying build.xml. I see two options: We create a bookkeeper/src/java/lib folder and add the netty jar. In this case, we edit build.xml to set its classpath appropriately; We make it use ivy and fetch from a repository. Another point is that for the new bookie classes, I haven't seen javadocs or comments at all. I think it would be great to have some in there.
          Hide
          Utkarsh Srivastava added a comment -

          Rewrite of bookkeeper client library

          Show
          Utkarsh Srivastava added a comment - Rewrite of bookkeeper client library
          Hide
          Utkarsh Srivastava added a comment -

          The new bookie classes are written by Ben. Ben, can you please add javadocs for those?

          Show
          Utkarsh Srivastava added a comment - The new bookie classes are written by Ben. Ben, can you please add javadocs for those?
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 36 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-h8.grid.sp2.yahoo.net/65/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/65/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/65/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/12422953/ZOOKEEPER-507.patch against trunk revision 835618. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 36 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-h8.grid.sp2.yahoo.net/65/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/65/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/65/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/12422953/ZOOKEEPER-507.patch
          against trunk revision 881623.

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

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

          ben,
          will you be adding javadocs to the classes?

          Show
          Mahadev konar added a comment - ben, will you be adding javadocs to the classes?
          Hide
          Flavio Junqueira added a comment -

          We also need to split this patch into two. There are really two major issue being solved here: new client code and ledger aggregation support.

          Show
          Flavio Junqueira added a comment - We also need to split this patch into two. There are really two major issue being solved here: new client code and ledger aggregation support.
          Hide
          Benjamin Reed added a comment -

          waiting for ben to update the bookie doc

          Show
          Benjamin Reed added a comment - waiting for ben to update the bookie doc
          Hide
          Flavio Junqueira added a comment -

          I have merged ZOOKEEPER-486 back. It doesn't seem to make a lot of sense to separate these patches because they are not completely disjoint. Aside from having more folks reviewing this patch, we need to:

          • Improve documentation (ZOOKEEPER-607);
          • Fetch netty jar through ivy (ZOOKEEPER-.633).
          Show
          Flavio Junqueira added a comment - I have merged ZOOKEEPER-486 back. It doesn't seem to make a lot of sense to separate these patches because they are not completely disjoint. Aside from having more folks reviewing this patch, we need to: Improve documentation ( ZOOKEEPER-607 ); Fetch netty jar through ivy (ZOOKEEPER-.633).
          Hide
          Flavio Junqueira added a comment -

          I had the wrong file name in the previous upload, I'm just uploading the same file with its name corrected.

          Show
          Flavio Junqueira added a comment - I had the wrong file name in the previous upload, I'm just uploading the same file with its name corrected.
          Hide
          Flavio Junqueira added a comment -

          BookieFailureTest is failing intermittently after the last modifications. I'm attaching the test output showing where it is hanging.

          Show
          Flavio Junqueira added a comment - BookieFailureTest is failing intermittently after the last modifications. I'm attaching the test output showing where it is hanging.
          Hide
          Flavio Junqueira added a comment -

          BookieFailureTest is failing consistently now, after I re-introduced the tests to check the failure of each bookie. These tests are redundant, but they should help to pinpoint problems that arise when the position of the bookie in the ensemble matters. A disadvantage of re-introducing the tests is that they increase the total time needed to run the tests.

          About the test failure, it seems that the test hangs when we error out add keys, but I'm not completely sure. I need to look more deeply into the problem.

          I have also wrote and modified javadoc blocks for some of the client package classes.

          Show
          Flavio Junqueira added a comment - BookieFailureTest is failing consistently now, after I re-introduced the tests to check the failure of each bookie. These tests are redundant, but they should help to pinpoint problems that arise when the position of the bookie in the ensemble matters. A disadvantage of re-introducing the tests is that they increase the total time needed to run the tests. About the test failure, it seems that the test hangs when we error out add keys, but I'm not completely sure. I need to look more deeply into the problem. I have also wrote and modified javadoc blocks for some of the client package classes.
          Hide
          Benjamin Reed added a comment -

          it turns out that the BookieFailureTest was failing due to a bug in netty. it appears to have been fixed at the end of september (see http://viewvc.jboss.org/cgi-bin/viewvc.cgi/netty/trunk/src/main/java/org/jboss/netty/channel/socket/nio/NioWorker.java?view=log) . the fix is in 3.1.5. Running with netty 3.1.5 allows all the tests to pass.

          Show
          Benjamin Reed added a comment - it turns out that the BookieFailureTest was failing due to a bug in netty. it appears to have been fixed at the end of september (see http://viewvc.jboss.org/cgi-bin/viewvc.cgi/netty/trunk/src/main/java/org/jboss/netty/channel/socket/nio/NioWorker.java?view=log ) . the fix is in 3.1.5. Running with netty 3.1.5 allows all the tests to pass.
          Hide
          Utkarsh Srivastava added a comment -

          Added documentation to user-facing classes. Made all the remaining non-user-facing classes package public.

          Show
          Utkarsh Srivastava added a comment - Added documentation to user-facing classes. Made all the remaining non-user-facing classes package public.
          Hide
          Hadoop QA added a comment -

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

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

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

          Utkarsh, This patch is broken. There is missing code, which you can actually see from the file size (compare to the previous patches). Could you upload a new complete patch?

          Show
          Flavio Junqueira added a comment - Utkarsh, This patch is broken. There is missing code, which you can actually see from the file size (compare to the previous patches). Could you upload a new complete patch?
          Hide
          Utkarsh Srivastava added a comment -

          Included all the new classes. Had forgot to svn add them earlier.

          Show
          Utkarsh Srivastava added a comment - Included all the new classes. Had forgot to svn add them earlier.
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 33 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 generated 211 release audit warnings (more than the trunk's current 207 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-h8.grid.sp2.yahoo.net/109/testReport/
          Release audit warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/109/artifact/trunk/patchprocess/releaseAuditDiffWarnings.txt
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/109/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/109/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/12430774/ZOOKEEPER-507.patch against trunk revision 899383. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 33 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 generated 211 release audit warnings (more than the trunk's current 207 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-h8.grid.sp2.yahoo.net/109/testReport/ Release audit warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/109/artifact/trunk/patchprocess/releaseAuditDiffWarnings.txt Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/109/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/109/console This message is automatically generated.
          Hide
          Flavio Junqueira added a comment -

          The following two classes are missing the license headers:

          1. BaseTestCase.java
          2. SyncCounter.java
          Show
          Flavio Junqueira added a comment - The following two classes are missing the license headers: BaseTestCase.java SyncCounter.java
          Hide
          Utkarsh Srivastava added a comment -

          Fixed warnings, added 2 missing license headers

          Show
          Utkarsh Srivastava added a comment - Fixed warnings, added 2 missing license headers
          Hide
          Hadoop QA added a comment -

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

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

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

          lets try and get this in 3.3.

          Show
          Mahadev konar added a comment - lets try and get this in 3.3.
          Hide
          Mahadev konar added a comment -

          utkarsh and ben,
          can you folks please update the jira saying

          • what changes have been made - the basic design change
          • what classes have been added and what is there purpose (better javadoc would help here)
          • what user facing changes have been made
          • are there forrest docs added for the user interface?
          • also i took a cursory look, it seems some (very few) of the classes do not have any javadoc or very minimal. It would be really helpful for future and current development if javadoc is added to these classes
          Show
          Mahadev konar added a comment - utkarsh and ben, can you folks please update the jira saying what changes have been made - the basic design change what classes have been added and what is there purpose (better javadoc would help here) what user facing changes have been made are there forrest docs added for the user interface? also i took a cursory look, it seems some (very few) of the classes do not have any javadoc or very minimal. It would be really helpful for future and current development if javadoc is added to these classes
          Hide
          Benjamin Reed added a comment -

          here are the main changes:

          1) changed to using netty for client requests
          2) made recovery more reliable
          3) multiplexed writes to handle a large number of concurrent ledgers being written to

          the user facing changes are minimal. our javadoc efforts have been with respect to user facing classes. the internal code is still a bit influx, so some of the internal or simple classes we have not put a lot of effort into documenting since they haven't really finalized.

          our main goal right now is to get the patch in since it is very large and hard to manage at this point.

          Show
          Benjamin Reed added a comment - here are the main changes: 1) changed to using netty for client requests 2) made recovery more reliable 3) multiplexed writes to handle a large number of concurrent ledgers being written to the user facing changes are minimal. our javadoc efforts have been with respect to user facing classes. the internal code is still a bit influx, so some of the internal or simple classes we have not put a lot of effort into documenting since they haven't really finalized. our main goal right now is to get the patch in since it is very large and hard to manage at this point.
          Hide
          Benjamin Reed added a comment -

          two extremely small fixes to make the tests patch:

          1) The syncThread was not being killed in the shutdown() method of Bookie
          2) We need to add pending ops before calling connect() in PerChannelBookieConnection because in linux with local host the connect will return immediately.

          we really need to commit this thing. the patch has become extremely unwieldy!

          Show
          Benjamin Reed added a comment - two extremely small fixes to make the tests patch: 1) The syncThread was not being killed in the shutdown() method of Bookie 2) We need to add pending ops before calling connect() in PerChannelBookieConnection because in linux with local host the connect will return immediately. we really need to commit this thing. the patch has become extremely unwieldy!
          Hide
          Utkarsh Srivastava added a comment -

          Ben, could you explain the change in PerChannelBookieClient.java. As far as I could tell, you exchanged the order of

          • initiating a connect, and
          • adding operation to pendingops

          But since these are done under a lock, and the connect callback grabs the same lock, switching this order is immaterial. It shouldn't have fixed anything.

          Show
          Utkarsh Srivastava added a comment - Ben, could you explain the change in PerChannelBookieClient.java. As far as I could tell, you exchanged the order of initiating a connect, and adding operation to pendingops But since these are done under a lock, and the connect callback grabs the same lock, switching this order is immaterial. It shouldn't have fixed anything.
          Hide
          Benjamin Reed added a comment -

          yeah that is exactly what i thought, which is why it took me so long to fix. the problem is that the connect in the same thread. so if the connect succeeds right away, which happens with local host, the pendingops list gets processed. thus, you need to add to pendingops before calling connect.

          Show
          Benjamin Reed added a comment - yeah that is exactly what i thought, which is why it took me so long to fix. the problem is that the connect in the same thread. so if the connect succeeds right away, which happens with local host, the pendingops list gets processed. thus, you need to add to pendingops before calling connect.
          Hide
          Utkarsh Srivastava added a comment -

          Oh, you mean the connect callback gets called in the same thread? If so, yes I can see why that would not work.

          Show
          Utkarsh Srivastava added a comment - Oh, you mean the connect callback gets called in the same thread? If so, yes I can see why that would not work.
          Hide
          Mahadev konar added a comment -

          +1 ... ill go ahead and commit this. please make sure you add more java docs and forrest docs for bookkeeper.

          Show
          Mahadev konar added a comment - +1 ... ill go ahead and commit this. please make sure you add more java docs and forrest docs for bookkeeper.
          Hide
          Mahadev konar added a comment -

          I just committed this. thanks utkarsh, ben and flavio.

          Show
          Mahadev konar added a comment - I just committed this. thanks utkarsh, ben and flavio.
          Hide
          Hudson added a comment -

          Integrated in ZooKeeper-trunk #680 (See http://hudson.zones.apache.org/hudson/job/ZooKeeper-trunk/680/)
          . BookKeeper client re-write (Utkarsh and ben via mahadev)

          Show
          Hudson added a comment - Integrated in ZooKeeper-trunk #680 (See http://hudson.zones.apache.org/hudson/job/ZooKeeper-trunk/680/ ) . BookKeeper client re-write (Utkarsh and ben via mahadev)

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development