Hadoop Common
  1. Hadoop Common
  2. HADOOP-6194

Add service base class and tests to hadoop-common/util

    Details

    • Type: New Feature New Feature
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Won't Fix
    • Affects Version/s: 0.21.0
    • Fix Version/s: None
    • Component/s: util
    • Labels:
      None

      Description

      For a service base class, we need to add to common

      1. The Service class
      2. A mock Service class (it's not in test, its in src/, because it helps in functional tests downstream)
      3. Unit tests

      This is the issue to cover this work

      1. find-new-warnings
        0.8 kB
        Aaron Kimball
      2. HADOOP-6194.patch
        52 kB
        Steve Loughran
      3. HADOOP-6194.patch
        43 kB
        Steve Loughran
      4. HADOOP-6194-1.patch
        62 kB
        steve_l

        Issue Links

          Activity

          Hide
          steve_l added a comment -

          This is the fraction of the service lifecycle that goes into common -the base class, a mock class for testing, and tests

          Show
          steve_l added a comment - This is the fraction of the service lifecycle that goes into common -the base class, a mock class for testing, and tests
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12417309/HADOOP-6194-1.patch
          against trunk revision 806745.

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

          +1 tests included. The patch appears to include 8 new or modified tests.

          -1 patch. The patch command could not apply the patch.

          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/622/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/12417309/HADOOP-6194-1.patch against trunk revision 806745. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 8 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/622/console This message is automatically generated.
          Hide
          Chris Douglas added a comment -

          I haven't been through the details, but the patch doesn't apply to trunk (req src/test/ and src/java/, not test/ and java/).

          This probably can't be committed until either HDFS-326, MAPREDUCE-233, or some other service lifecycle issue has been ironed out, right? With the freeze for 0.21 in mid-September, it's optimistic to pre-commit base functionality without at least one issue ready to commit. Whether this is left PA or not is up to you

          Show
          Chris Douglas added a comment - I haven't been through the details, but the patch doesn't apply to trunk (req src/test/ and src/java/, not test/ and java/). This probably can't be committed until either HDFS-326 , MAPREDUCE-233 , or some other service lifecycle issue has been ironed out, right? With the freeze for 0.21 in mid-September, it's optimistic to pre-commit base functionality without at least one issue ready to commit. Whether this is left PA or not is up to you
          Hide
          Steve Loughran added a comment -

          This is the most recent version of the lifecycle/service base class for hadoop-common

          1. Named HadoopService so that the Service in the security package doesn't cause confusion
          2. the protected methods are serviceStart and {{serviceClose{{
          3. Anything purely of interest to those nodes that have workers is in the abstract subclass HadoopServiceWithWorkers. Currently there's just one abstract method to return the no of workers, though I'd like there to be a standard way to get the list of workers and ports and last reported time, which we could then feed to management tools and a standard servlet that would report this up. That is future work, these base classes just set things up.
          Show
          Steve Loughran added a comment - This is the most recent version of the lifecycle/service base class for hadoop-common Named HadoopService so that the Service in the security package doesn't cause confusion the protected methods are serviceStart and {{serviceClose{{ Anything purely of interest to those nodes that have workers is in the abstract subclass HadoopServiceWithWorkers . Currently there's just one abstract method to return the no of workers, though I'd like there to be a standard way to get the list of workers and ports and last reported time, which we could then feed to management tools and a standard servlet that would report this up. That is future work, these base classes just set things up.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12442895/HADOOP-6194.patch
          against trunk revision 938136.

          +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 appears to have generated 1 warning messages.

          -1 javac. The applied patch generated 1026 javac compiler warnings (more than the trunk's current 1024 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/Hadoop-Patch-h4.grid.sp2.yahoo.net/481/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/481/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/481/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/481/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/12442895/HADOOP-6194.patch against trunk revision 938136. +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 appears to have generated 1 warning messages. -1 javac. The applied patch generated 1026 javac compiler warnings (more than the trunk's current 1024 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/Hadoop-Patch-h4.grid.sp2.yahoo.net/481/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/481/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/481/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/481/console This message is automatically generated.
          Hide
          Tom White added a comment -

          Some comments on the latest patch:

          • Naming: maybe "LifecycleService" instead of "HadoopService"? "MockService" could be "MockLifecycleService".
          • Also, just "State" instead of "ServiceState", so the full form would be "LifecycleService.State". Some methods already refer to "State", while others refer to "ServiceState", so this change would help make this more regular.
          • Javadoc on line 44 of HadoopService is malformed.
          • It's unclear what the unit test is testing - the code in HadoopService, or the code in MockService, or both? In general, mocks are used as doubles for testing logic in the real class. Can the code be changed to make the distinction clearer?
          • As Chris pointed out, it would be good to know that a real service daemon works with this base code.
          Show
          Tom White added a comment - Some comments on the latest patch: Naming: maybe "LifecycleService" instead of "HadoopService"? "MockService" could be "MockLifecycleService". Also, just "State" instead of "ServiceState", so the full form would be "LifecycleService.State". Some methods already refer to "State", while others refer to "ServiceState", so this change would help make this more regular. Javadoc on line 44 of HadoopService is malformed. It's unclear what the unit test is testing - the code in HadoopService, or the code in MockService, or both? In general, mocks are used as doubles for testing logic in the real class. Can the code be changed to make the distinction clearer? As Chris pointed out, it would be good to know that a real service daemon works with this base code.
          Hide
          Steve Loughran added a comment -

          This is an updated patch which

          • uses the name LifecycleService for the class, LifecycleServiceWithWorkers for the subclass intended for services with workers.
          • added much more javadocs and even some more test cases to the test

          I'll stick up the HDFS services tomorrow; I will synchronise and run all the tests. The big changes there are primarily in adding more shutdown logic to remove all threads; this could be teased out to make the lifecycle patch look smaller.

          I have a patch for the JT and TT too; the only troublespot there is in startup, where I believe it can block forever waiting for the filesystem to go live in startup. There's also MAPREDUCE-437. These are separate issues from how you start and stop the things, just needed to make sure that when you start or stop them they clean up and don't ever block waiting for some external dependency like a functional filesystem.

          Show
          Steve Loughran added a comment - This is an updated patch which uses the name LifecycleService for the class, LifecycleServiceWithWorkers for the subclass intended for services with workers. added much more javadocs and even some more test cases to the test I'll stick up the HDFS services tomorrow; I will synchronise and run all the tests. The big changes there are primarily in adding more shutdown logic to remove all threads; this could be teased out to make the lifecycle patch look smaller. I have a patch for the JT and TT too; the only troublespot there is in startup, where I believe it can block forever waiting for the filesystem to go live in startup. There's also MAPREDUCE-437 . These are separate issues from how you start and stop the things, just needed to make sure that when you start or stop them they clean up and don't ever block waiting for some external dependency like a functional filesystem.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12443007/HADOOP-6194.patch
          against trunk revision 938590.

          +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 generated 1026 javac compiler warnings (more than the trunk's current 1024 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/Hadoop-Patch-h1.grid.sp2.yahoo.net/56/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h1.grid.sp2.yahoo.net/56/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h1.grid.sp2.yahoo.net/56/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h1.grid.sp2.yahoo.net/56/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/12443007/HADOOP-6194.patch against trunk revision 938590. +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 generated 1026 javac compiler warnings (more than the trunk's current 1024 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/Hadoop-Patch-h1.grid.sp2.yahoo.net/56/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h1.grid.sp2.yahoo.net/56/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h1.grid.sp2.yahoo.net/56/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h1.grid.sp2.yahoo.net/56/console This message is automatically generated.
          Hide
          Steve Loughran added a comment -

          I've submitted a patch to the HDFS services under HDFS-326 which is in sync with this to show it works

          Now, how do you find out javac warnings that don't appear to get printed to the hudson console?

          Show
          Steve Loughran added a comment - I've submitted a patch to the HDFS services under HDFS-326 which is in sync with this to show it works Now, how do you find out javac warnings that don't appear to get printed to the hudson console?
          Hide
          Aaron Kimball added a comment -

          Steve, I've attached a bash script that does some awk + diff magic on the trunk javac warnings and patch javac warnings output to attempt to discover the new ones. It does a reasonable job (at the very least, it should tell you which files to look at).

          Show
          Aaron Kimball added a comment - Steve, I've attached a bash script that does some awk + diff magic on the trunk javac warnings and patch javac warnings output to attempt to discover the new ones. It does a reasonable job (at the very least, it should tell you which files to look at).
          Hide
          Steve Loughran added a comment -

          thanks Aaron, I will look at that script.

          I have just stuck up onto MAPREDUCE-233 the patch that brings the TT and JT under the lifecycle. The JT is the tricky one, as it likes to wait for the filesystem to be live first since the JT added persistence; I think nobody has noticed that other than me as their filesystems come up more reliably than mine, but the problem is there in trunk too.

          Show
          Steve Loughran added a comment - thanks Aaron, I will look at that script. I have just stuck up onto MAPREDUCE-233 the patch that brings the TT and JT under the lifecycle. The JT is the tricky one, as it likes to wait for the filesystem to be live first since the JT added persistence; I think nobody has noticed that other than me as their filesystems come up more reliably than mine, but the problem is there in trunk too.
          Hide
          Tom White added a comment -

          I just came across the Service class in the Guava library, which looks like it does something very similar. Would it be worth considering using this?

          Interestingly, there's also a @Beta annotation, which is like InterfaceStability.Unstable.

          Show
          Tom White added a comment - I just came across the Service class in the Guava library, which looks like it does something very similar. Would it be worth considering using this? Interestingly, there's also a @Beta annotation, which is like InterfaceStability.Unstable.
          Hide
          Steve Loughran added a comment -

          Looks similar, as their basic lifecycle set of states matches fairly well, they have a stopping state to describe slow shutdowns which could make sense. It's the same philosophy of how you design a class to represent a subclassable service with a lifecycle that can be aggregated.

          Their base class itself is a bit minimal, I like to record why something failed. "Failed" isn't enough, exceptions are handy. I'd also like it to implement Closeable as that's generally handy.

          If consensus was to adopt the guava lib as yet-another-dependency, there's no reason why their AbstractServiceBase couldn't be used, but I'd still want a HadoopServiceBase under this which would be our base class and have extra methods in there.

          However, we don't need to rush to Guava just because we would gain a lot from having our own service base class, be it our own or descended from the Guava one.

          Show
          Steve Loughran added a comment - Looks similar, as their basic lifecycle set of states matches fairly well, they have a stopping state to describe slow shutdowns which could make sense. It's the same philosophy of how you design a class to represent a subclassable service with a lifecycle that can be aggregated. Their base class itself is a bit minimal, I like to record why something failed. "Failed" isn't enough, exceptions are handy. I'd also like it to implement Closeable as that's generally handy. If consensus was to adopt the guava lib as yet-another-dependency, there's no reason why their AbstractServiceBase couldn't be used, but I'd still want a HadoopServiceBase under this which would be our base class and have extra methods in there. However, we don't need to rush to Guava just because we would gain a lot from having our own service base class, be it our own or descended from the Guava one.
          Hide
          Steve Loughran added a comment -

          one other thing to consider is that the jetty instances that HDFS and the JT brings up are a service too, and they should be manageable as well, as are, effectively the in-JVM HDFS and MR clusters. I haven't gone near that code, yet, but it would make things consistent.

          Show
          Steve Loughran added a comment - one other thing to consider is that the jetty instances that HDFS and the JT brings up are a service too, and they should be manageable as well, as are, effectively the in-JVM HDFS and MR clusters. I haven't gone near that code, yet, but it would make things consistent.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12443133/find-new-warnings
          against trunk revision 1031422.

          +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 new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

          -1 patch. The patch command could not apply the patch.

          Console output: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/71//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/12443133/find-new-warnings against trunk revision 1031422. +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 new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. -1 patch. The patch command could not apply the patch. Console output: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/71//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/12443133/find-new-warnings
          against trunk revision 1071364.

          +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 new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

          -1 patch. The patch command could not apply the patch.

          Console output: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/290//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/12443133/find-new-warnings against trunk revision 1071364. +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 new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. -1 patch. The patch command could not apply the patch. Console output: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/290//console This message is automatically generated.
          Hide
          Todd Lipcon added a comment -

          Appears to be out of date with trunk

          Show
          Todd Lipcon added a comment - Appears to be out of date with trunk
          Hide
          Steve Loughran added a comment -

          superceded by the YARN services, though work needs to be done there to make its lifecycle more robust. the YARN service lifecycle could also be moved into hadoop-common at some point

          Show
          Steve Loughran added a comment - superceded by the YARN services, though work needs to be done there to make its lifecycle more robust. the YARN service lifecycle could also be moved into hadoop-common at some point

            People

            • Assignee:
              Steve Loughran
              Reporter:
              Steve Loughran
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development