Details

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

      Description

      We can also have an option to start the Autorecovery along with Bookie servers.
      If some users are not having too much load on the servers, they can even start them along the Bookie servers. If they feel, Auditor would disturb Bookie performance, they can anyway start as separate process.

      In another case, deployment overhead will reduce a bit as Monitoring process need not monitor one more process in their lifcycles etc.

      Thoughts?

      1. 0001-BOOKKEEPER-472-Provide-an-option-to-start-Autorecove.patch
        29 kB
        Ivan Kelly
      2. BOOKKEEPER-472-remove-trailing-spaces.patch
        28 kB
        Uma Maheswara Rao G
      3. BOOKKEEPER-472-3.patch
        28 kB
        Uma Maheswara Rao G
      4. BOOKKEEPER-472-2.patch
        26 kB
        Uma Maheswara Rao G
      5. BOOKKEEPER-472-1.patch
        25 kB
        Uma Maheswara Rao G
      6. BOOKKEEPER-472.patch
        25 kB
        Uma Maheswara Rao G

        Activity

        Hide
        Flavio Junqueira added a comment -

        Uma Maheswara Rao G Hi Uma, You're the assignee of this task. Do you think you'll be able to provide a patch? Has it been implemented anywhere else?

        Show
        Flavio Junqueira added a comment - Uma Maheswara Rao G Hi Uma, You're the assignee of this task. Do you think you'll be able to provide a patch? Has it been implemented anywhere else?
        Hide
        Uma Maheswara Rao G added a comment -

        This patch almost ready, I am doing some basic test and will upload it tomorrow!

        Show
        Uma Maheswara Rao G added a comment - This patch almost ready, I am doing some basic test and will upload it tomorrow!
        Hide
        Uma Maheswara Rao G added a comment -

        Attached an initial patch!

        Show
        Uma Maheswara Rao G added a comment - Attached an initial patch!
        Hide
        Hadoop QA added a comment -

        Testing JIRA BOOKKEEPER-472

        Patch BOOKKEEPER-472.patch downloaded at Mon Dec 17 16:11:27 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 contains 18 line(s) with trailing spaces
        . -1 the patch contains 6 line(s) longer than 120 characters
        . +1 the patch does adds/modifies 4 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 does not compile
        . +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 - patch does not compile, cannot run testcases
        -1 DISTRO
        . -1 distro tarball fails 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/135/

        Show
        Hadoop QA added a comment - Testing JIRA BOOKKEEPER-472 Patch BOOKKEEPER-472.patch downloaded at Mon Dec 17 16:11:27 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 contains 18 line(s) with trailing spaces . -1 the patch contains 6 line(s) longer than 120 characters . +1 the patch does adds/modifies 4 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 does not compile . +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 - patch does not compile, cannot run testcases -1 DISTRO . -1 distro tarball fails 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/135/
        Hide
        Ivan Kelly added a comment -

        Uma Maheswara Rao G I think this would be better done at the script level, to avoid having autorecovery in the same jvm process. I think isolating like this is better for stability. How I imagined this, is that you have an variable in conf/bkenv.sh or in conf/bk_server.conf, which bin/bookkeeper-daemon.sh uses to decide whether to start a autorecovery alongside bookie or not.

        Show
        Ivan Kelly added a comment - Uma Maheswara Rao G I think this would be better done at the script level, to avoid having autorecovery in the same jvm process. I think isolating like this is better for stability. How I imagined this, is that you have an variable in conf/bkenv.sh or in conf/bk_server.conf, which bin/bookkeeper-daemon.sh uses to decide whether to start a autorecovery alongside bookie or not.
        Hide
        Uma Maheswara Rao G added a comment -

        Thanks a lot, Ivan for taking a look.
        Actually my intention here is to keep them in same JVM. Right now I may not encourage to run them in same jvm, mey be due to stability issues as you mentioned. But we are interested to run them as single process once autorecovery stabilized in future, otherwise OM need to monitor 2 processes instead of 1. So, this is currently just an optional and not documented yet.
        Also our initial version of autorecovery is running along with Bookie process itself and going good. Also deployment perspective in NN HA is:
        2 NN, 2 ZKFC, 3 ZKs, 3 BKs + ( 3 Autorecovery processes) = 13 processes required to have NN HA.
        This is just an optional, if we wanted to run them separately also we have that option as default.

        Show
        Uma Maheswara Rao G added a comment - Thanks a lot, Ivan for taking a look. Actually my intention here is to keep them in same JVM. Right now I may not encourage to run them in same jvm, mey be due to stability issues as you mentioned. But we are interested to run them as single process once autorecovery stabilized in future, otherwise OM need to monitor 2 processes instead of 1. So, this is currently just an optional and not documented yet. Also our initial version of autorecovery is running along with Bookie process itself and going good. Also deployment perspective in NN HA is: 2 NN, 2 ZKFC, 3 ZKs, 3 BKs + ( 3 Autorecovery processes) = 13 processes required to have NN HA. This is just an optional, if we wanted to run them separately also we have that option as default.
        Hide
        Ivan Kelly added a comment -

        Regarding in-process/out-of-process, im ok with the former for now. If it turns out to be problematic, we can change it.

        I have a few comments about the patch. Apart from these it's ready to go

        1. rename the config option to "autoRecoveryDaemonEnabled". Theres a typo in it now.
        2. ServerConfiguration#setAutoRecoveryDaemonEnabled() should take a boolean parameter.
        3. BookieServer#autoRecoveryMain is never set to null initially.
        4. Typo in name of AutoRecoveryMain#isAutoRecoveryRunning
        Show
        Ivan Kelly added a comment - Regarding in-process/out-of-process, im ok with the former for now. If it turns out to be problematic, we can change it. I have a few comments about the patch. Apart from these it's ready to go rename the config option to "autoRecoveryDaemonEnabled". Theres a typo in it now. ServerConfiguration#setAutoRecoveryDaemonEnabled() should take a boolean parameter. BookieServer#autoRecoveryMain is never set to null initially. Typo in name of AutoRecoveryMain#isAutoRecoveryRunning
        Hide
        Uma Maheswara Rao G added a comment -

        Thanks a lot, Ivan for the review!
        I have updated the patch by addressing the comments above. Pls take a look.

        Show
        Uma Maheswara Rao G added a comment - Thanks a lot, Ivan for the review! I have updated the patch by addressing the comments above. Pls take a look.
        Hide
        Hadoop QA added a comment -

        Testing JIRA BOOKKEEPER-472

        Patch BOOKKEEPER-472-1.patch downloaded at Wed Jan 2 19:08:47 UTC 2013

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

        +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 contains 16 line(s) with trailing spaces
        . +1 the patch does not introduce any line longer than 120
        . +1 the patch does adds/modifies 4 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 does not compile
        . +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 - patch does not compile, cannot run testcases
        -1 DISTRO
        . -1 distro tarball fails 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/187/

        Show
        Hadoop QA added a comment - Testing JIRA BOOKKEEPER-472 Patch BOOKKEEPER-472-1.patch downloaded at Wed Jan 2 19:08:47 UTC 2013 ---------------------------- +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 contains 16 line(s) with trailing spaces . +1 the patch does not introduce any line longer than 120 . +1 the patch does adds/modifies 4 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 does not compile . +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 - patch does not compile, cannot run testcases -1 DISTRO . -1 distro tarball fails 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/187/
        Hide
        Ivan Kelly added a comment -

        Looks like LocalBookKeeper needs the exceptions added. I'm guessing you didn't clean before compile

        Show
        Ivan Kelly added a comment - Looks like LocalBookKeeper needs the exceptions added. I'm guessing you didn't clean before compile
        Hide
        Flavio Junqueira added a comment -

        Please upload a new patch, Uma.

        Show
        Flavio Junqueira added a comment - Please upload a new patch, Uma.
        Hide
        Uma Maheswara Rao G added a comment -

        Yeah, I missed it somehow. I have just updated a new one which solves that compilation issue.

        Show
        Uma Maheswara Rao G added a comment - Yeah, I missed it somehow. I have just updated a new one which solves that compilation issue.
        Hide
        Hadoop QA added a comment -

        Testing JIRA BOOKKEEPER-472

        Patch BOOKKEEPER-472-2.patch downloaded at Thu Jan 3 06:32:12 UTC 2013

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

        +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 contains 17 line(s) with trailing spaces
        . +1 the patch does not introduce any line longer than 120
        . +1 the patch does adds/modifies 4 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 does not compile
        . +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 - patch does not compile, cannot run testcases
        -1 DISTRO
        . -1 distro tarball fails 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/189/

        Show
        Hadoop QA added a comment - Testing JIRA BOOKKEEPER-472 Patch BOOKKEEPER-472-2.patch downloaded at Thu Jan 3 06:32:12 UTC 2013 ---------------------------- +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 contains 17 line(s) with trailing spaces . +1 the patch does not introduce any line longer than 120 . +1 the patch does adds/modifies 4 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 does not compile . +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 - patch does not compile, cannot run testcases -1 DISTRO . -1 distro tarball fails 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/189/
        Hide
        Uma Maheswara Rao G added a comment -

        Hedwig tests extended some methods directly. there also I have just added exceptions now.

        Show
        Uma Maheswara Rao G added a comment - Hedwig tests extended some methods directly. there also I have just added exceptions now.
        Hide
        Hadoop QA added a comment -

        Testing JIRA BOOKKEEPER-472

        Patch BOOKKEEPER-472-3.patch downloaded at Thu Jan 3 07:31:13 UTC 2013

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

        +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 contains 14 line(s) with trailing spaces
        . +1 the patch does not introduce any line longer than 120
        . +1 the patch does adds/modifies 5 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: 760
        +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/191/

        Show
        Hadoop QA added a comment - Testing JIRA BOOKKEEPER-472 Patch BOOKKEEPER-472-3.patch downloaded at Thu Jan 3 07:31:13 UTC 2013 ---------------------------- +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 contains 14 line(s) with trailing spaces . +1 the patch does not introduce any line longer than 120 . +1 the patch does adds/modifies 5 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: 760 +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/191/
        Hide
        Uma Maheswara Rao G added a comment -

        Just removing the trailing spaces. There is no code change in this patch compared to previous one. Eclipse formatter is not able to remove trailing spaces automatically in javadocs.

        Show
        Uma Maheswara Rao G added a comment - Just removing the trailing spaces. There is no code change in this patch compared to previous one. Eclipse formatter is not able to remove trailing spaces automatically in javadocs.
        Hide
        Hadoop QA added a comment -

        Testing JIRA BOOKKEEPER-472

        Patch BOOKKEEPER-472-remove-trailing-spaces.patch downloaded at Thu Jan 3 09:21:12 UTC 2013

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

        +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 contains 5 line(s) with trailing spaces
        . +1 the patch does not introduce any line longer than 120
        . +1 the patch does adds/modifies 5 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: 760
        +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/192/

        Show
        Hadoop QA added a comment - Testing JIRA BOOKKEEPER-472 Patch BOOKKEEPER-472-remove-trailing-spaces.patch downloaded at Thu Jan 3 09:21:12 UTC 2013 ---------------------------- +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 contains 5 line(s) with trailing spaces . +1 the patch does not introduce any line longer than 120 . +1 the patch does adds/modifies 5 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: 760 +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/192/
        Hide
        Ivan Kelly added a comment -

        Committed as r1428258. Thanks Uma.

        Show
        Ivan Kelly added a comment - Committed as r1428258. Thanks Uma.
        Hide
        Ivan Kelly added a comment -

        This never went into trunk. Pushing it in now. (the svn commit revision was for BOOKKEEPER-409)

        Show
        Ivan Kelly added a comment - This never went into trunk. Pushing it in now. (the svn commit revision was for BOOKKEEPER-409 )
        Hide
        Ivan Kelly added a comment -

        Committed r1432964(trunk) & r1432966(4.2 branch). Thanks Uma

        Show
        Ivan Kelly added a comment - Committed r1432964(trunk) & r1432966(4.2 branch). Thanks Uma
        Hide
        Ivan Kelly added a comment -

        Attaching finally committed patch. It needed some conflicts resolved.

        Show
        Ivan Kelly added a comment - Attaching finally committed patch. It needed some conflicts resolved.
        Hide
        Hudson added a comment -

        Integrated in bookkeeper-trunk2 #61 (See https://builds.apache.org/job/bookkeeper-trunk2/61/)
        BOOKKEEPER-472: Provide an option to start Autorecovery along with Bookie Servers (umamahesh via ivank) (Revision 1432964)

        Result = SUCCESS
        ivank :
        Files :

        • /zookeeper/bookkeeper/trunk/CHANGES.txt
        • /zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ServerConfiguration.java
        • /zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieServer.java
        • /zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/replication/AutoRecoveryMain.java
        • /zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/util/LocalBookKeeper.java
        • /zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/ReplicationTestUtil.java
        • /zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/TestAutoRecoveryAlongWithBookieServers.java
        • /zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/TestReplicationWorker.java
        • /zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookKeeperClusterTestCase.java
        • /zookeeper/bookkeeper/trunk/hedwig-server/src/test/java/org/apache/hedwig/server/persistence/BookKeeperTestBase.java
        Show
        Hudson added a comment - Integrated in bookkeeper-trunk2 #61 (See https://builds.apache.org/job/bookkeeper-trunk2/61/ ) BOOKKEEPER-472 : Provide an option to start Autorecovery along with Bookie Servers (umamahesh via ivank) (Revision 1432964) Result = SUCCESS ivank : Files : /zookeeper/bookkeeper/trunk/CHANGES.txt /zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ServerConfiguration.java /zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieServer.java /zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/replication/AutoRecoveryMain.java /zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/util/LocalBookKeeper.java /zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/ReplicationTestUtil.java /zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/TestAutoRecoveryAlongWithBookieServers.java /zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/TestReplicationWorker.java /zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookKeeperClusterTestCase.java /zookeeper/bookkeeper/trunk/hedwig-server/src/test/java/org/apache/hedwig/server/persistence/BookKeeperTestBase.java

          People

          • Assignee:
            Uma Maheswara Rao G
            Reporter:
            Uma Maheswara Rao G
          • Votes:
            0 Vote for this issue
            Watchers:
            7 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development