Bookkeeper
  1. Bookkeeper
  2. BOOKKEEPER-181 Scale hedwig
  3. BOOKKEEPER-205

implement a MetaStore based ledger manager for bookkeeper client.

    Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.2.0
    • Component/s: bookkeeper-client
    • Labels:
      None

      Description

      implement a MetaStore based ledger manager for bookkeeper client.

        Issue Links

          Activity

          Sijie Guo created issue -
          Sijie Guo made changes -
          Field Original Value New Value
          Link This issue is blocked by BOOKKEEPER-204 [ BOOKKEEPER-204 ]
          Sijie Guo made changes -
          Link This issue is blocked by BOOKKEEPER-203 [ BOOKKEEPER-203 ]
          Jiannan Wang made changes -
          Assignee Jiannan Wang [ jiannan ]
          Hide
          Jiannan Wang added a comment -

          Attach an implementation patch. Also post it in review board:
          https://reviews.apache.org/r/8035/

          Show
          Jiannan Wang added a comment - Attach an implementation patch. Also post it in review board: https://reviews.apache.org/r/8035/
          Jiannan Wang made changes -
          Attachment BOOKKEEPER-205.diff [ 12553268 ]
          Jiannan Wang made changes -
          Fix Version/s 4.2.0 [ 12320244 ]
          Jiannan Wang made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Hide
          Hadoop QA added a comment -

          Testing JIRA BOOKKEEPER-205

          WARNING: Running test-patch on a dirty local svn workspace

          Patch <a href="/jira/secure/attachment/12553268/BOOKKEEPER-205.diff">/jira/secure/attachment/12553268/BOOKKEEPER-205.diff</a> downloaded at Tue Nov 27 12:52:52 UTC 2012

          ----------------------------

          +1 PATCH_APPLIES
          +1 CLEAN
          -1 RAW_PATCH_ANALYSIS
          . +1 the patch does not introduce any @author tags
          . +1 the patch does not introduce any tabs
          . +1 the patch does not introduce any trailing spaces
          . -1 the patch contains 3 line(s) longer than 120 characters
          . +1 the patch does adds/modifies 6 testcase(s)
          +1 RAT
          . +1 the patch does not seem to introduce new RAT warnings
          +1 JAVADOC
          . +1 the patch does not seem to introduce new Javadoc warnings
          . WARNING: the current HEAD has 8 Javadoc warning(s)
          +1 COMPILE
          . +1 HEAD compiles
          . +1 patch compiles
          . +1 the patch does not seem to introduce new javac warnings
          . WARNING: the current HEAD has 9 javac warning(s)
          +1 FINDBUGS
          . +1 the patch does not seem to introduce new Findbugs warnings
          . WARNING: the current HEAD has Findbugs warning(s), they should be addressed ASAP
          -1 TESTS
          . Tests run: 479
          . Tests failed: 2
          . Tests errors: 0

          . The patch failed the following testcases:

          . testLedgerDelete[2](org.apache.bookkeeper.test.LedgerDeleteTest)
          . testLedgerDeleteWithExistingEntryLogs[2](org.apache.bookkeeper.test.LedgerDeleteTest)

          +1 DISTRO
          . +1 distro tarball builds with the patch

          ----------------------------
          -1 Overall result, please check the reported -1(s)

          . There is at least one warning, please check

          The full output of the test-patch run is available at

          . https://builds.apache.org/job/bookkeeper-trunk-precommit-build/35/

          Show
          Hadoop QA added a comment - Testing JIRA BOOKKEEPER-205 WARNING: Running test-patch on a dirty local svn workspace Patch <a href="/jira/secure/attachment/12553268/ BOOKKEEPER-205 .diff">/jira/secure/attachment/12553268/ BOOKKEEPER-205 .diff</a> downloaded at Tue Nov 27 12:52:52 UTC 2012 ---------------------------- +1 PATCH_APPLIES +1 CLEAN -1 RAW_PATCH_ANALYSIS . +1 the patch does not introduce any @author tags . +1 the patch does not introduce any tabs . +1 the patch does not introduce any trailing spaces . -1 the patch contains 3 line(s) longer than 120 characters . +1 the patch does adds/modifies 6 testcase(s) +1 RAT . +1 the patch does not seem to introduce new RAT warnings +1 JAVADOC . +1 the patch does not seem to introduce new Javadoc warnings . WARNING : the current HEAD has 8 Javadoc warning(s) +1 COMPILE . +1 HEAD compiles . +1 patch compiles . +1 the patch does not seem to introduce new javac warnings . WARNING : the current HEAD has 9 javac warning(s) +1 FINDBUGS . +1 the patch does not seem to introduce new Findbugs warnings . WARNING: the current HEAD has Findbugs warning(s), they should be addressed ASAP -1 TESTS . Tests run: 479 . Tests failed: 2 . Tests errors: 0 . The patch failed the following testcases: . testLedgerDelete [2] (org.apache.bookkeeper.test.LedgerDeleteTest) . testLedgerDeleteWithExistingEntryLogs [2] (org.apache.bookkeeper.test.LedgerDeleteTest) +1 DISTRO . +1 distro tarball builds with the patch ---------------------------- -1 Overall result, please check the reported -1(s) . There is at least one warning, please check The full output of the test-patch run is available at . https://builds.apache.org/job/bookkeeper-trunk-precommit-build/35/
          Hide
          Ivan Kelly added a comment -

          I'm not sure about this patch. It creates another variant for tests, which will increase the test time by 50%. They already take too long. For this reason, I'd like get BOOKKEEPER-474 and BOOKKEEPER-475 in before this one goes in.

          In terms of code, generally it looks fine. I'd rename MetastoreClass to MetastoreImplementationClass or MetastoreImplClass. Also, the javadoc should mention that the configuration options are only used it Metastore ledger manager is enabled in the other config items.

          Also, there's a failing test in the precommit build, and something went in recently which is messing with the patch application.

          Show
          Ivan Kelly added a comment - I'm not sure about this patch. It creates another variant for tests, which will increase the test time by 50%. They already take too long. For this reason, I'd like get BOOKKEEPER-474 and BOOKKEEPER-475 in before this one goes in. In terms of code, generally it looks fine. I'd rename MetastoreClass to MetastoreImplementationClass or MetastoreImplClass. Also, the javadoc should mention that the configuration options are only used it Metastore ledger manager is enabled in the other config items. Also, there's a failing test in the precommit build, and something went in recently which is messing with the patch application.
          Ivan Kelly made changes -
          Status Patch Available [ 10002 ] Open [ 1 ]
          Hide
          Jiannan Wang added a comment -

          I'm not sure about this patch. It creates another variant for tests, which will increase the test time by 50%.


          Yeah, it's a problem. I don't know if it's possible to specify LedgerManagerFactory classes in Maven command parameter to improve it. Actually, we only test for HierarchicalLedgerManagerFactory and MSLedgerManagerFactory in our inner usage.

          In terms of code, generally it looks fine. I'd rename MetastoreClass to MetastoreImplementationClass or MetastoreImplClass. Also, the javadoc should mention that the configuration options are only used it Metastore ledger manager is enabled in the other config items.
          -------
          Will change.

          Also, there's a failing test in the precommit build, and something went in recently which is messing with the patch application.
          -------
          I'll check it.

          Show
          Jiannan Wang added a comment - I'm not sure about this patch. It creates another variant for tests, which will increase the test time by 50%. Yeah, it's a problem. I don't know if it's possible to specify LedgerManagerFactory classes in Maven command parameter to improve it. Actually, we only test for HierarchicalLedgerManagerFactory and MSLedgerManagerFactory in our inner usage. In terms of code, generally it looks fine. I'd rename MetastoreClass to MetastoreImplementationClass or MetastoreImplClass. Also, the javadoc should mention that the configuration options are only used it Metastore ledger manager is enabled in the other config items. ------- Will change. Also, there's a failing test in the precommit build, and something went in recently which is messing with the patch application. ------- I'll check it.
          Hide
          Ivan Kelly added a comment -

          I have a few comments in review board also.

          Show
          Ivan Kelly added a comment - I have a few comments in review board also.
          Hide
          Jiannan Wang added a comment -

          Attach a new patch

          Show
          Jiannan Wang added a comment - Attach a new patch
          Jiannan Wang made changes -
          Attachment BOOKKEEPER-205.diff [ 12559840 ]
          Hide
          Jiannan Wang added a comment -

          Test Total time that without MSLedgerManagerFactory: 14:59.787s
          Test Total time that involve MSLedgerManagerFactory: 19:29.503s

          Show
          Jiannan Wang added a comment - Test Total time that without MSLedgerManagerFactory: 14:59.787s Test Total time that involve MSLedgerManagerFactory: 19:29.503s
          Jiannan Wang made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Hide
          Ivan Kelly added a comment -

          Patch causes a fairly consistent failure in AuditorLedgerCheckerTest. I'm attaching the log. It doesn't happen every time, but it happens often.

          Show
          Ivan Kelly added a comment - Patch causes a fairly consistent failure in AuditorLedgerCheckerTest. I'm attaching the log. It doesn't happen every time, but it happens often.
          Ivan Kelly made changes -
          Status Patch Available [ 10002 ] Open [ 1 ]
          Ivan Kelly made changes -
          Ivan Kelly made changes -
          Link This issue is blocked by BOOKKEEPER-496 [ BOOKKEEPER-496 ]
          Hide
          Ivan Kelly added a comment -

          Failure seems to be a problem in the auditor elector, and how its using watchers. I think this patch just aggravates the issue, but it does cause regular failures. I'll fix the watcher misuse in BOOKKEEPER-496 as its related to those changes already.

          Show
          Ivan Kelly added a comment - Failure seems to be a problem in the auditor elector, and how its using watchers. I think this patch just aggravates the issue, but it does cause regular failures. I'll fix the watcher misuse in BOOKKEEPER-496 as its related to those changes already.
          Hide
          Jiannan Wang added a comment -

          From the log, the test failed when starting ZooKeeper cluster. However, I don't think my change will affect these code, I'll check it.

          Show
          Jiannan Wang added a comment - From the log, the test failed when starting ZooKeeper cluster. However, I don't think my change will affect these code, I'll check it.
          Hide
          Jiannan Wang added a comment -

          Oh, I didn't refresh the page before posting comment, ignore my comment and thanks Ivan for troubleshooting.

          Show
          Jiannan Wang added a comment - Oh, I didn't refresh the page before posting comment, ignore my comment and thanks Ivan for troubleshooting.
          Hide
          Ivan Kelly added a comment -

          The issue is in MSLedgerManagerFactory.MsLedgerManager#doAsyncProcessLedgers.

          // no entries now
          if (!cursor.hasMoreEntries()) {
              finalCb.processResult(successRc, null, context);
          }
          

          Should be

          // no entries now
          if (!cursor.hasMoreEntries()) {
              finalCb.processResult(successRc, null, context);
              return;
          }
          

          without the return, it's going into a loop and eating CPU, which is causing the other things on the machine to be starved, hence the ZK session timeouts.

          Show
          Ivan Kelly added a comment - The issue is in MSLedgerManagerFactory.MsLedgerManager#doAsyncProcessLedgers. // no entries now if (!cursor.hasMoreEntries()) { finalCb.processResult(successRc, null , context); } Should be // no entries now if (!cursor.hasMoreEntries()) { finalCb.processResult(successRc, null , context); return ; } without the return, it's going into a loop and eating CPU, which is causing the other things on the machine to be starved, hence the ZK session timeouts.
          Ivan Kelly made changes -
          Hide
          Ivan Kelly added a comment -

          New patch includes fix.

          Show
          Ivan Kelly added a comment - New patch includes fix.
          Ivan Kelly made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Hide
          Hadoop QA added a comment -

          Testing JIRA BOOKKEEPER-205

          Patch 0001-BOOKKEEPER-205-implement-a-MetaStore-based-ledger-ma.patch downloaded at Tue Dec 11 23:31:47 UTC 2012

          ----------------------------

          +1 PATCH_APPLIES
          +1 CLEAN
          -1 RAW_PATCH_ANALYSIS
          . +1 the patch does not introduce any @author tags
          . +1 the patch does not introduce any tabs
          . +1 the patch does not introduce any trailing spaces
          . -1 the patch contains 2 line(s) longer than 120 characters
          . +1 the patch does adds/modifies 6 testcase(s)
          +1 RAT
          . +1 the patch does not seem to introduce new RAT warnings
          +1 JAVADOC
          . +1 the patch does not seem to introduce new Javadoc warnings
          +1 COMPILE
          . +1 HEAD compiles
          . +1 patch compiles
          . +1 the patch does not seem to introduce new javac warnings
          +1 FINDBUGS
          . +1 the patch does not seem to introduce new Findbugs warnings
          +1 TESTS
          . Tests run: 479
          +1 DISTRO
          . +1 distro tarball builds with the patch

          ----------------------------
          -1 Overall result, please check the reported -1(s)

          The full output of the test-patch run is available at

          . https://builds.apache.org/job/bookkeeper-trunk-precommit-build/99/

          Show
          Hadoop QA added a comment - Testing JIRA BOOKKEEPER-205 Patch 0001-BOOKKEEPER-205-implement-a-MetaStore-based-ledger-ma.patch downloaded at Tue Dec 11 23:31:47 UTC 2012 ---------------------------- +1 PATCH_APPLIES +1 CLEAN -1 RAW_PATCH_ANALYSIS . +1 the patch does not introduce any @author tags . +1 the patch does not introduce any tabs . +1 the patch does not introduce any trailing spaces . -1 the patch contains 2 line(s) longer than 120 characters . +1 the patch does adds/modifies 6 testcase(s) +1 RAT . +1 the patch does not seem to introduce new RAT warnings +1 JAVADOC . +1 the patch does not seem to introduce new Javadoc warnings +1 COMPILE . +1 HEAD compiles . +1 patch compiles . +1 the patch does not seem to introduce new javac warnings +1 FINDBUGS . +1 the patch does not seem to introduce new Findbugs warnings +1 TESTS . Tests run: 479 +1 DISTRO . +1 distro tarball builds with the patch ---------------------------- -1 Overall result, please check the reported -1(s) The full output of the test-patch run is available at . https://builds.apache.org/job/bookkeeper-trunk-precommit-build/99/
          Hide
          Sijie Guo added a comment -

          thanks for Ivan fixing the infinite loop. +1 for the new patch.

          Show
          Sijie Guo added a comment - thanks for Ivan fixing the infinite loop. +1 for the new patch.
          Hide
          Ivan Kelly added a comment -

          Committed as r1420607. Good work Jiannan.

          I fixed up the long lines when committing. Attaching final patch.

          Show
          Ivan Kelly added a comment - Committed as r1420607. Good work Jiannan. I fixed up the long lines when committing. Attaching final patch.
          Ivan Kelly made changes -
          Status Patch Available [ 10002 ] Resolved [ 5 ]
          Resolution Fixed [ 1 ]
          Ivan Kelly made changes -
          Ivan Kelly made changes -
          Link This issue is blocked by BOOKKEEPER-496 [ BOOKKEEPER-496 ]
          Ivan Kelly made changes -
          Link This issue is duplicated by BOOKKEEPER-206 [ BOOKKEEPER-206 ]
          Hide
          Jiannan Wang added a comment -

          Thanks Ivan a lot for finding the bug and making patch.

          Show
          Jiannan Wang added a comment - Thanks Ivan a lot for finding the bug and making patch.
          Hide
          Hudson added a comment -

          Integrated in bookkeeper-trunk #856 (See https://builds.apache.org/job/bookkeeper-trunk/856/)
          BOOKKEEPER-205: implement a MetaStore based ledger manager for bookkeeper client. (jiannan via ivank) (Revision 1420607)

          Result = UNSTABLE
          ivank :
          Files :

          • /zookeeper/bookkeeper/trunk/CHANGES.txt
          • /zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BKException.java
          • /zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/AbstractConfiguration.java
          • /zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MSLedgerManagerFactory.java
          • /zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieRecoveryTest.java
          • /zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/metastore/TestMetaStore.java
          • /zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/BookieLedgerIndexTest.java
          • /zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookKeeperClusterTestCase.java
          • /zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/test/MultiLedgerManagerMultiDigestTestCase.java
          • /zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/test/MultiLedgerManagerTestCase.java
          Show
          Hudson added a comment - Integrated in bookkeeper-trunk #856 (See https://builds.apache.org/job/bookkeeper-trunk/856/ ) BOOKKEEPER-205 : implement a MetaStore based ledger manager for bookkeeper client. (jiannan via ivank) (Revision 1420607) Result = UNSTABLE ivank : Files : /zookeeper/bookkeeper/trunk/CHANGES.txt /zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BKException.java /zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/AbstractConfiguration.java /zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MSLedgerManagerFactory.java /zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieRecoveryTest.java /zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/metastore/TestMetaStore.java /zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/BookieLedgerIndexTest.java /zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookKeeperClusterTestCase.java /zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/test/MultiLedgerManagerMultiDigestTestCase.java /zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/test/MultiLedgerManagerTestCase.java
          Ivan Kelly made changes -
          Status Resolved [ 5 ] Closed [ 6 ]

            People

            • Assignee:
              Jiannan Wang
              Reporter:
              Sijie Guo
            • Votes:
              1 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development