Hadoop Common
  1. Hadoop Common
  2. HADOOP-5257

Export namenode/datanode functionality through a pluggable RPC layer

    Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.21.0
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    • Release Note:
      New plugin facility for namenode and datanode instantiates classes named in new configuration properties dfs.datanode.plugins and dfs.namenode.plugins.

      Description

      Adding support for pluggable components would allow exporting DFS functionallity using arbitrary protocols, like Thirft or Protocol Buffers. I'm opening this issue on Dhruba's suggestion in HADOOP-4707.

      Plug-in implementations would extend this base class:

      abstract class Plugin {
      
          public abstract datanodeStarted(DataNode datanode);
      
          public abstract datanodeStopping();
      
          public abstract namenodeStarted(NameNode namenode);
      
          public abstract namenodeStopping();
      }

      Name node instances would then start the plug-ins according to a configuration object, and would also shut them down when the node goes down:

      public class NameNode {
      
          // [..]
      
          private void initialize(Configuration conf)
              // [...]
              for (Plugin p: PluginManager.loadPlugins(conf))
                p.namenodeStarted(this);
          }
      
          // [..]
      
          public void stop() {
              if (stopRequested)
                  return;
              stopRequested = true;
              for (Plugin p: plugins) 
                  p.namenodeStopping();
              // [..]
          }
      
          // [..]
      }

      Data nodes would do a similar thing in DataNode.startDatanode() and DataNode.shutdown

      1. HADOOP-5257.patch
        20 kB
        Carlos Valiente
      2. HADOOP-5257-v2.patch
        17 kB
        Carlos Valiente
      3. HADOOP-5257-v3.patch
        16 kB
        Carlos Valiente
      4. HADOOP-5257-v4.patch
        15 kB
        Carlos Valiente
      5. HADOOP-5257-v5.patch
        11 kB
        Carlos Valiente
      6. HADOOP-5257-v6.patch
        11 kB
        Carlos Valiente
      7. HADOOP-5257-v7.patch
        11 kB
        Carlos Valiente
      8. HADOOP-5257-v8.patch
        11 kB
        Carlos Valiente

        Issue Links

          Activity

          Carlos Valiente created issue -
          Carlos Valiente made changes -
          Field Original Value New Value
          Attachment HADOOP-5257.patch [ 12400201 ]
          dhruba borthakur made changes -
          Link This issue blocks HADOOP-4707 [ HADOOP-4707 ]
          Hide
          Doug Cutting added a comment -

          It seems likely we'll want plugins elsewhere, so the manager might be more abstract, able to return instances of other interfaces determined by different configuration properties. This might go into core. Then we might have separate datanode and namenode plugin interfaces and configuration properties.

          Show
          Doug Cutting added a comment - It seems likely we'll want plugins elsewhere, so the manager might be more abstract, able to return instances of other interfaces determined by different configuration properties. This might go into core. Then we might have separate datanode and namenode plugin interfaces and configuration properties.
          Hide
          dhruba borthakur added a comment -

          @Doug: yes, we can make the plugin more general and make it part of org.apache.hadoop.util package. Can you pl enumerate the APIs (other than start/stop) that this interface might provide?

          @Carlos: the namenode/datanode plugin interface javadoc may want to document that the plugins are started after the namenode is started and is stopped before the namenode is stopped.

          Show
          dhruba borthakur added a comment - @Doug: yes, we can make the plugin more general and make it part of org.apache.hadoop.util package. Can you pl enumerate the APIs (other than start/stop) that this interface might provide? @Carlos: the namenode/datanode plugin interface javadoc may want to document that the plugins are started after the namenode is started and is stopped before the namenode is stopped.
          Hide
          Doug Cutting added a comment -

          > Can you pl enumerate the APIs (other than start/stop) that this interface might provide?

          The generic plugin manager/factory should know nothing of the interface. So the API to get plugins might look like:

          List<MyInterface> plugins = PluginFactory.getPlugins(conf, "my.config.property", MyInterface.class);

          This might even be a Configuration method, e.g.:

          List<MyInterface> plugins = conf.getInstances("my.config.property", MyInterface.class);

          It can be written fairly straightforwardly using Configuration#getClasses() and ReflectionUtils#newInstance.

          Show
          Doug Cutting added a comment - > Can you pl enumerate the APIs (other than start/stop) that this interface might provide? The generic plugin manager/factory should know nothing of the interface. So the API to get plugins might look like: List<MyInterface> plugins = PluginFactory.getPlugins(conf, "my.config.property", MyInterface.class); This might even be a Configuration method, e.g.: List<MyInterface> plugins = conf.getInstances("my.config.property", MyInterface.class); It can be written fairly straightforwardly using Configuration#getClasses() and ReflectionUtils#newInstance.
          Hide
          Carlos Valiente added a comment -

          Thanks for the feedback, guys. HADOOP-5257-v2.patch implements Configuration.getInstances(), as suggested by Doug. Datanode and namenode interfaces are now specified by two distinct classes, and the start and stop order is mentioned in both clasess' javadocs, as suggested by Dhruba.

          Show
          Carlos Valiente added a comment - Thanks for the feedback, guys. HADOOP-5257 -v2.patch implements Configuration.getInstances() , as suggested by Doug. Datanode and namenode interfaces are now specified by two distinct classes, and the start and stop order is mentioned in both clasess' javadocs, as suggested by Dhruba.
          Carlos Valiente made changes -
          Attachment HADOOP-5257-v2.patch [ 12400660 ]
          Hide
          Carlos Valiente added a comment -

          NamenodePlugin and DatanodePlugin are now interfaces, instead of abstract classes.

          Show
          Carlos Valiente added a comment - NamenodePlugin and DatanodePlugin are now interfaces, instead of abstract classes.
          Carlos Valiente made changes -
          Attachment HADOOP-5257-v3.patch [ 12400692 ]
          Hide
          dhruba borthakur added a comment -

          This looks good.

          Since these are kinda-APIs to namenode/datanode, they should probably reside in src/hdfs/org/apache/hadoop/hdfs/server/protocol. Also, there could be just one Interface (instead of two), both the namenode and datanode should be able to use the same interface. The Interface would have two methods, start and stop. Please let me know if this sounds ok to you.

          Show
          dhruba borthakur added a comment - This looks good. Since these are kinda-APIs to namenode/datanode, they should probably reside in src/hdfs/org/apache/hadoop/hdfs/server/protocol. Also, there could be just one Interface (instead of two), both the namenode and datanode should be able to use the same interface. The Interface would have two methods, start and stop. Please let me know if this sounds ok to you.
          Hide
          Carlos Valiente added a comment -

          Since these are kinda-APIs to namenode/datanode, they should probably reside in src/hdfs/org/apache/hadoop/hdfs/server/protocol.

          Attachement HADOOP-5257-v4.patch includes that, Dhruba.

          Also, there could be just one Interface (instead of two), both the namenode and datanode should be able to use the same interface. The Interface would have two methods, start and stop.

          Included as well:

          public interface Plugin {
          
            public void start(Object server);
          
            public void stop();
          }
          
          Show
          Carlos Valiente added a comment - Since these are kinda-APIs to namenode/datanode, they should probably reside in src/hdfs/org/apache/hadoop/hdfs/server/protocol. Attachement HADOOP-5257 -v4.patch includes that, Dhruba. Also, there could be just one Interface (instead of two), both the namenode and datanode should be able to use the same interface. The Interface would have two methods, start and stop. Included as well: public interface Plugin { public void start( Object server); public void stop(); }
          Carlos Valiente made changes -
          Attachment HADOOP-5257-v4.patch [ 12402198 ]
          Hide
          Konstantin Shvachko added a comment -

          Minor comments:

          1. DFS should be HDFS
          2. It is not clear from the name TestGetInstances what it tests and there is no JavaDoc explaining it.
          3. It would be better if classes ChildInterface, SampleInterface, AnotherClass, SampleClass were static subclasses of TestGetInstances.
          Show
          Konstantin Shvachko added a comment - Minor comments: DFS should be HDFS It is not clear from the name TestGetInstances what it tests and there is no JavaDoc explaining it. It would be better if classes ChildInterface, SampleInterface, AnotherClass, SampleClass were static subclasses of TestGetInstances.
          Hide
          Carlos Valiente added a comment -

          Thanks for your feedback, Konstantin

          1. DFS should be HDFS

          Yep - done in HADOOP-5257-v4.patch

          2. It is not clear from the name TestGetInstances what it tests and there is no JavaDoc explaining it.

          I've added a javadoc comment explaining the purpose of the test.

          3. It would be better if classes ChildInterface, SampleInterface, AnotherClass, SampleClass were static subclasses of TestGetInstances.

          Done.

          While modifying the javadoc for org.apache.hadoop.hdfs.server.protocol.Plugin, I've noticed there's nothing HDFS-specific to that interface - should it perhaps be moved to some place under src/core instead?

          Show
          Carlos Valiente added a comment - Thanks for your feedback, Konstantin 1. DFS should be HDFS Yep - done in HADOOP-5257 -v4.patch 2. It is not clear from the name TestGetInstances what it tests and there is no JavaDoc explaining it. I've added a javadoc comment explaining the purpose of the test. 3. It would be better if classes ChildInterface, SampleInterface, AnotherClass, SampleClass were static subclasses of TestGetInstances. Done. While modifying the javadoc for org.apache.hadoop.hdfs.server.protocol.Plugin , I've noticed there's nothing HDFS-specific to that interface - should it perhaps be moved to some place under src/core instead?
          Carlos Valiente made changes -
          Attachment HADOOP-5257-v4.patch [ 12402309 ]
          Carlos Valiente made changes -
          Attachment HADOOP-5257-v4.patch [ 12402309 ]
          Hide
          Carlos Valiente added a comment -

          Oops, wrong file - HADOOP-5257-v5.patch is the right one

          Show
          Carlos Valiente added a comment - Oops, wrong file - HADOOP-5257 -v5.patch is the right one
          Carlos Valiente made changes -
          Attachment HADOOP-5257-v5.patch [ 12402311 ]
          Hide
          dhruba borthakur added a comment -

          > While modifying the javadoc for org.apache.hadoop.hdfs.server.protocol.Plugin, I've noticed there's nothing HDFS-specific to that interface - should it perhaps be moved to some place under src/core instead?

          Yes, that sounds right. It seems like a good idea to move it to core.

          Show
          dhruba borthakur added a comment - > While modifying the javadoc for org.apache.hadoop.hdfs.server.protocol.Plugin, I've noticed there's nothing HDFS-specific to that interface - should it perhaps be moved to some place under src/core instead? Yes, that sounds right. It seems like a good idea to move it to core.
          Hide
          Carlos Valiente added a comment -

          Yes, that sounds right. It seems like a good idea to move it to core.

          Waht about org.apache.hadoop.util.Plugin? There's already a MemoryCalculatorPlugin and a LinuxMemoryCalculatorPlugin in that package.

          Show
          Carlos Valiente added a comment - Yes, that sounds right. It seems like a good idea to move it to core. Waht about org.apache.hadoop.util.Plugin ? There's already a MemoryCalculatorPlugin and a LinuxMemoryCalculatorPlugin in that package.
          Hide
          Konstantin Shvachko added a comment -

          Moving Plugin to core means that you will not be able to call HDFS specific methods in unit tests you will probably write later for testing.
          I don't know whether you will need that, just a consideration to take into account.

          Show
          Konstantin Shvachko added a comment - Moving Plugin to core means that you will not be able to call HDFS specific methods in unit tests you will probably write later for testing. I don't know whether you will need that, just a consideration to take into account.
          Hide
          Carlos Valiente added a comment -

          Moving Plugin to core means that you will not be able to call HDFS specific methods in unit tests you will probably write later for testing.

          My idea was to move just the Plugin interface to core, and keep the implementation classes in HADOOP-4707 (and their unit tests) under the appropriate HDFS packages — but perhaps it's just simpler to leave Plugin where it lives now. Any suggestions?

          Show
          Carlos Valiente added a comment - Moving Plugin to core means that you will not be able to call HDFS specific methods in unit tests you will probably write later for testing. My idea was to move just the Plugin interface to core, and keep the implementation classes in HADOOP-4707 (and their unit tests) under the appropriate HDFS packages — but perhaps it's just simpler to leave Plugin where it lives now. Any suggestions?
          Hide
          Doug Cutting added a comment -

          Plugin might better be called ServicePlugin, since, while not HDFS-specific, it is specific to services that are started and stopped. If it were so renamed, I'd support moving it to src/core, where it might nicely complement HADOOP-3628.

          Show
          Doug Cutting added a comment - Plugin might better be called ServicePlugin, since, while not HDFS-specific, it is specific to services that are started and stopped. If it were so renamed, I'd support moving it to src/core, where it might nicely complement HADOOP-3628 .
          Hide
          Carlos Valiente added a comment -

          Plugin might better be called ServicePlugin [..]

          Done in HADOOP-5257-v6.patch, Doug — and moved to org.apache.hadoop.util under src/core. I also added some null checks on DataNode.shutdown() and NameNode.stop() to fix several test failures.

          'ant test-core' and 'ant test-patch' succeed.

          Show
          Carlos Valiente added a comment - Plugin might better be called ServicePlugin [..] Done in HADOOP-5257 -v6.patch, Doug — and moved to org.apache.hadoop.util under src/core . I also added some null checks on DataNode.shutdown() and NameNode.stop() to fix several test failures. 'ant test-core' and 'ant test-patch' succeed.
          Carlos Valiente made changes -
          Attachment HADOOP-5257-v6.patch [ 12402418 ]
          Hide
          Carlos Valiente added a comment -

          Minor fixes for 'ant checkstyle'

          Show
          Carlos Valiente added a comment - Minor fixes for 'ant checkstyle'
          Carlos Valiente made changes -
          Attachment HADOOP-5257-v7.patch [ 12402576 ]
          Carlos Valiente made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Release Note Added support for HDFS pluggable components.
          Hide
          steve_l added a comment -

          1. Looking at the code, the plugin interface is very like the service lifecycle in HADOOP-3628; start, stop, and such like. There's no reason why we couldn't use the same lifecycle interface for plugins as the services themselves; they just push their children through the same lifecycle. The ping() operation could be used to aggregate the state of the children; you could even model the http and ipc servers similarly. I've been contemplating doing this for MiniMRCluster.

          2. when you aggregate, you soon reach the policy questions: how to handle failure on startup. This patch just warns and continues. I may not want my name nodes to start up if they cant start a critical plugin; having it drop to the failed state with a stack trace that management tools can see. At the very least, the warn/fail choice should be an option for the various nodes; having plugin-specific policy is feature creep.

          3. Also: configuration. How do these plugins get configured. Are they expected to sneak a look at their parent's configuration, or should the parent pass down a specific configuration for them?

          4. Finally if the plugin interface extends Closeable(), it would make it possible for us to have utility code to handle sets of closeable items for cleanup, usable in lots of other places too

          Show
          steve_l added a comment - 1. Looking at the code, the plugin interface is very like the service lifecycle in HADOOP-3628 ; start, stop, and such like. There's no reason why we couldn't use the same lifecycle interface for plugins as the services themselves; they just push their children through the same lifecycle. The ping() operation could be used to aggregate the state of the children; you could even model the http and ipc servers similarly. I've been contemplating doing this for MiniMRCluster. 2. when you aggregate, you soon reach the policy questions: how to handle failure on startup. This patch just warns and continues. I may not want my name nodes to start up if they cant start a critical plugin; having it drop to the failed state with a stack trace that management tools can see. At the very least, the warn/fail choice should be an option for the various nodes; having plugin-specific policy is feature creep. 3. Also: configuration. How do these plugins get configured. Are they expected to sneak a look at their parent's configuration, or should the parent pass down a specific configuration for them? 4. Finally if the plugin interface extends Closeable(), it would make it possible for us to have utility code to handle sets of closeable items for cleanup, usable in lots of other places too
          Hide
          Carlos Valiente added a comment -

          Thanks for your comments, Steve.

          1. Looking at the code, the plugin interface is very like the service lifecycle in HADOOP-3628; start, stop, and such like. There's no reason why we couldn't use the same lifecycle interface for plugins as the services themselves

          Sounds very reasonable. Just one thing: For my current prototype of the Thrift Namenode plugin, I use the reference to the o.a.h.hdfs.server.namenode.NameNode instance passed as the parameter to the ServicePlugin.start(server). Something like this:

          class NamenodePlugin implements ServicePlugin {
            
            NameNode namenode;
          
            void start(Object server) {
              this.namenode = (NameNode)server;
            }
          
            // [..]
          
            /* A sample method of the Thrift interface */
            public void enterSafeMode() {
              namenode.setSafeMode(SafeModeAction.SAFEMODE_ENTER);
            }
          

          How could that be achieved in the context of HADOOP-3628?

          All in all I'm happy to base this patch on HADOOP-3628. Are you aiming for 0.21 with your stuff, Steve? If HADOOP-3628 were to be included in 0.21, that would make things very easy for my Thrift stuff.

          2. when you aggregate, you soon reach the policy questions: how to handle failure on startup. This patch just warns and continues. I may not want my name nodes to start up if they cant start a critical plugin

          What about having configurable options, like dfs.namenode.plugin.failonerror, and dfs.dataenode.plugin.failonerror, so that each service may decide what to do?

          3. Also: configuration. How do these plugins get configured. Are they expected to sneak a look at their parent's configuration [..]

          In its current form, yes, they may have a look at their parent's config (since they get a reference to the parent in the call to ServicePlugin.start(server). Each plugin may also look for configuration objects somewhere else via Configuration.addResource().

          In the context of HADOOP-3628 I suppose their parents would use something like plugin.setConf(this.conf).

          4. Finally if the plugin interface extends Closeable(), it would make it possible for us to have utility code to handle sets of closeable items for cleanup, usable in lots of other places too.

          Good idea

          Show
          Carlos Valiente added a comment - Thanks for your comments, Steve. 1. Looking at the code, the plugin interface is very like the service lifecycle in HADOOP-3628 ; start, stop, and such like. There's no reason why we couldn't use the same lifecycle interface for plugins as the services themselves Sounds very reasonable. Just one thing: For my current prototype of the Thrift Namenode plugin, I use the reference to the o.a.h.hdfs.server.namenode.NameNode instance passed as the parameter to the ServicePlugin.start(server) . Something like this: class NamenodePlugin implements ServicePlugin { NameNode namenode; void start( Object server) { this .namenode = (NameNode)server; } // [..] /* A sample method of the Thrift interface */ public void enterSafeMode() { namenode.setSafeMode(SafeModeAction.SAFEMODE_ENTER); } How could that be achieved in the context of HADOOP-3628 ? All in all I'm happy to base this patch on HADOOP-3628 . Are you aiming for 0.21 with your stuff, Steve? If HADOOP-3628 were to be included in 0.21, that would make things very easy for my Thrift stuff. 2. when you aggregate, you soon reach the policy questions: how to handle failure on startup. This patch just warns and continues. I may not want my name nodes to start up if they cant start a critical plugin What about having configurable options, like dfs.namenode.plugin.failonerror , and dfs.dataenode.plugin.failonerror , so that each service may decide what to do? 3. Also: configuration. How do these plugins get configured. Are they expected to sneak a look at their parent's configuration [..] In its current form, yes, they may have a look at their parent's config (since they get a reference to the parent in the call to ServicePlugin.start(server) . Each plugin may also look for configuration objects somewhere else via Configuration.addResource() . In the context of HADOOP-3628 I suppose their parents would use something like plugin.setConf(this.conf) . 4. Finally if the plugin interface extends Closeable(), it would make it possible for us to have utility code to handle sets of closeable items for cleanup, usable in lots of other places too. Good idea
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12402576/HADOOP-5257-v7.patch
          against trunk revision 756256.

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

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

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs warnings.

          +1 Eclipse classpath. The patch retains Eclipse classpath integrity.

          +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-vesta.apache.org/112/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/112/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/112/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/112/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/12402576/HADOOP-5257-v7.patch against trunk revision 756256. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 Eclipse classpath. The patch retains Eclipse classpath integrity. +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-vesta.apache.org/112/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/112/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/112/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/112/console This message is automatically generated.
          Hide
          dhruba borthakur added a comment -

          +1 for this code.

          Steve: do you see this patch going into trunk (0.21) without stepping on the toes for HADOOP-3628?

          Show
          dhruba borthakur added a comment - +1 for this code. Steve: do you see this patch going into trunk (0.21) without stepping on the toes for HADOOP-3628 ?
          dhruba borthakur made changes -
          Link This issue relates to HADOOP-3199 [ HADOOP-3199 ]
          Hide
          steve_l added a comment -

          @Druba. The stuff could co-exist, I'm just thinking about how to do it most cleanly

          1. Making all these things Closeable() would be handy, as there is utility stuff common to lots of things that could be used (lists of closeable components). some existing helper classes used by the various nodes could also be made closeable, for a single way to shut things down.
          2. I'm unsure about the failure handling here when there is a chain of plugged in things. Today failures on shutdown are logged and ignored, startup is trickier to handle, and helper classes that get started by a node may not get shut down cleanly if something goes wrong in the startup chain after it. (this is something I think I handle in HADOOP-3628)
          3. We could use the ping operation in HADOOP-3628, tease it out to its own interface, and then any of these plugins that implement the health check would be pulled in to the node's health check.
          4. If the plugins took an implementation of Service in their constructor, then the plugins would have the ability to call Configured.getConf() to get the configuration, and any other bits of the standard API. I'm not sure they would need to, not having a plugin.
          5. We could extend the MockService to do some plugin testing, start them with the lifecycle, roll them back etc.
          Show
          steve_l added a comment - @Druba. The stuff could co-exist, I'm just thinking about how to do it most cleanly Making all these things Closeable() would be handy, as there is utility stuff common to lots of things that could be used (lists of closeable components). some existing helper classes used by the various nodes could also be made closeable, for a single way to shut things down. I'm unsure about the failure handling here when there is a chain of plugged in things. Today failures on shutdown are logged and ignored, startup is trickier to handle, and helper classes that get started by a node may not get shut down cleanly if something goes wrong in the startup chain after it. (this is something I think I handle in HADOOP-3628 ) We could use the ping operation in HADOOP-3628 , tease it out to its own interface, and then any of these plugins that implement the health check would be pulled in to the node's health check. If the plugins took an implementation of Service in their constructor, then the plugins would have the ability to call Configured.getConf() to get the configuration, and any other bits of the standard API. I'm not sure they would need to, not having a plugin. We could extend the MockService to do some plugin testing, start them with the lifecycle, roll them back etc.
          Hide
          dhruba borthakur added a comment -

          > 1. Making all these things Closeable() would be handy,

          This makes sense. Carlos: would you be able to make this change to the current patch?

          > 2. I'm unsure about the failure handling here when there is a chain of plugged in things.

          I agree. How about if we keep it simple and say that it will be logged. There is no guarantee on the order of plugins startup sequence either.

          > 3. We could use the ping operation in HADOOP-3628,
          > 4. If the plugins took an implementation of Service in their constructor,...
          > 5. We could extend the MockService to do some plugin

          The above three things woudl be great, maybe we can defer these three things till HADOOP-3628 makes it to trunk?

          Show
          dhruba borthakur added a comment - > 1. Making all these things Closeable() would be handy, This makes sense. Carlos: would you be able to make this change to the current patch? > 2. I'm unsure about the failure handling here when there is a chain of plugged in things. I agree. How about if we keep it simple and say that it will be logged. There is no guarantee on the order of plugins startup sequence either. > 3. We could use the ping operation in HADOOP-3628 , > 4. If the plugins took an implementation of Service in their constructor,... > 5. We could extend the MockService to do some plugin The above three things woudl be great, maybe we can defer these three things till HADOOP-3628 makes it to trunk?
          Hide
          Carlos Valiente added a comment -

          This makes sense. Carlos: would you be able to make this change to the current patch?

          ServicePlugin in patch HADOOP-5257-v8.patch now extends java.io.Closeable.

          Show
          Carlos Valiente added a comment - This makes sense. Carlos: would you be able to make this change to the current patch? ServicePlugin in patch HADOOP-5257 -v8.patch now extends java.io.Closeable .
          Carlos Valiente made changes -
          Attachment HADOOP-5257-v8.patch [ 12403497 ]
          Hide
          steve_l added a comment -

          @Dhruba
          yes, we can defer the others until later.

          Other points

          1. which classloader is being used to load classes?
          2. If parsing a string value to a list is useful, this should really go into Configuration, not the plugin classes, as that is one places to implement string trim policy, write the unit tests, etc.
          3. I like the tests, add one to try loading a class that isn't there
          Show
          steve_l added a comment - @Dhruba yes, we can defer the others until later. Other points which classloader is being used to load classes? If parsing a string value to a list is useful, this should really go into Configuration, not the plugin classes, as that is one places to implement string trim policy, write the unit tests, etc. I like the tests, add one to try loading a class that isn't there
          Hide
          steve_l added a comment -

          My other issue is that there are some very hard coded assumption that this code is limited to the filesystem. As doug says, a ServicePlugin would be more generic. You'd have to have separate classes for the NamenodeServicePlugin and the DatanodeServicePlugin, but you wouldn't need more interfaces when you came to add support for TaskTracker, JobTracker, Pig, etc.

          Show
          steve_l added a comment - My other issue is that there are some very hard coded assumption that this code is limited to the filesystem. As doug says, a ServicePlugin would be more generic. You'd have to have separate classes for the NamenodeServicePlugin and the DatanodeServicePlugin , but you wouldn't need more interfaces when you came to add support for TaskTracker, JobTracker, Pig, etc.
          Hide
          Carlos Valiente added a comment -

          1. which classloader is being used to load classes?

          Classes are loaded by Configuration.getInstances, which ends up calling Configuration.getClassByName, which uses the instance field Configuration.classloader. That field is initialised by this code fragment:

            private ClassLoader classLoader;
            {
              classLoader = Thread.currentThread().getContextClassLoader();
              if (classLoader == null) {
                classLoader = Configuration.class.getClassLoader();
              }
            }
          

          2. If parsing a string value to a list is useful, this should really go into Configuration, not the plugin classes, as that is one places to implement string trim policy, write the unit tests, etc.

          I'm not sure I follow you on this point, Steve: Class name parsing is delegated to Configuration.getClasses already (which delegates the splitting to StringUtils.getStrings, it seems).

          3. I like the tests, add one to try loading a class that isn't there

          org.apache.hadoop.conf.TestGetInstances already does that:

              try {
                conf.setStrings("some.classes",
                    SampleClass.class.getName(), AnotherClass.class.getName(),
                    "no.such.Class");
                conf.getInstances("some.classes", SampleInterface.class);
                fail("no.such.Class does not exist");
              } catch (RuntimeException e) {}
          

          Do you think it would be better to write it in a different way?

          Show
          Carlos Valiente added a comment - 1. which classloader is being used to load classes? Classes are loaded by Configuration.getInstances , which ends up calling Configuration.getClassByName , which uses the instance field Configuration.classloader . That field is initialised by this code fragment: private ClassLoader classLoader; { classLoader = Thread .currentThread().getContextClassLoader(); if (classLoader == null ) { classLoader = Configuration.class.getClassLoader(); } } 2. If parsing a string value to a list is useful, this should really go into Configuration, not the plugin classes, as that is one places to implement string trim policy, write the unit tests, etc. I'm not sure I follow you on this point, Steve: Class name parsing is delegated to Configuration.getClasses already (which delegates the splitting to StringUtils.getStrings , it seems). 3. I like the tests, add one to try loading a class that isn't there org.apache.hadoop.conf.TestGetInstances already does that: try { conf.setStrings( "some.classes" , SampleClass.class.getName(), AnotherClass.class.getName(), "no.such. Class " ); conf.getInstances( "some.classes" , SampleInterface.class); fail( "no.such. Class does not exist" ); } catch (RuntimeException e) {} Do you think it would be better to write it in a different way?
          Hide
          Carlos Valiente added a comment -

          My other issue is that there are some very hard coded assumption that this code is limited to the filesystem. As doug says, a ServicePlugin would be more generic. You'd have to have separate classes for the NamenodeServicePlugin and the DatanodeServicePlugin, but you wouldn't need more interfaces when you came to add support for TaskTracker, JobTracker, Pig, etc.

          The attachment HADOOP-5257-v8.patch already addresses that, I think

          Show
          Carlos Valiente added a comment - My other issue is that there are some very hard coded assumption that this code is limited to the filesystem. As doug says, a ServicePlugin would be more generic. You'd have to have separate classes for the NamenodeServicePlugin and the DatanodeServicePlugin, but you wouldn't need more interfaces when you came to add support for TaskTracker, JobTracker, Pig, etc. The attachment HADOOP-5257-v8.patch already addresses that, I think
          Hide
          dhruba borthakur added a comment -

          Cancelling patch to re-trigger hudsonqa tests

          Show
          dhruba borthakur added a comment - Cancelling patch to re-trigger hudsonqa tests
          dhruba borthakur made changes -
          Status Patch Available [ 10002 ] Open [ 1 ]
          Hide
          dhruba borthakur added a comment -

          Trigger hadoopQA tests

          Show
          dhruba borthakur added a comment - Trigger hadoopQA tests
          dhruba borthakur made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12403497/HADOOP-5257-v8.patch
          against trunk revision 759932.

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

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

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs warnings.

          +1 Eclipse classpath. The patch retains Eclipse classpath integrity.

          +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 failed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-minerva.apache.org/79/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-minerva.apache.org/79/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-minerva.apache.org/79/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-minerva.apache.org/79/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/12403497/HADOOP-5257-v8.patch against trunk revision 759932. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 Eclipse classpath. The patch retains Eclipse classpath integrity. +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 failed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-minerva.apache.org/79/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-minerva.apache.org/79/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-minerva.apache.org/79/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-minerva.apache.org/79/console This message is automatically generated.
          Hide
          Carlos Valiente added a comment -

          I don't think test failures are related to this patch:

          java.lang.NullPointerException
          	at org.apache.hadoop.mapred.JobInProgress.terminateJob(JobInProgress.java:2117)
          	at org.apache.hadoop.mapred.JobInProgress.terminate(JobInProgress.java:2153)
          	at org.apache.hadoop.mapred.JobInProgress.kill(JobInProgress.java:2221)
          	at org.apache.hadoop.mapred.TestCapacityScheduler$FakeTaskTrackerManager.killJob(TestCapacityScheduler.java:359)
          	at org.apache.hadoop.mapred.TestCapacityScheduler.testClusterBlockingForLackOfMemory(TestCapacityScheduler.java:2069)
          

          and:

          java.lang.NullPointerException
          	at org.apache.hadoop.mapred.JobInProgress.terminateJob(JobInProgress.java:2117)
          	at org.apache.hadoop.mapred.JobInProgress.terminate(JobInProgress.java:2153)
          	at org.apache.hadoop.mapred.JobInProgress.kill(JobInProgress.java:2221)
          	at org.apache.hadoop.mapred.TestCapacityScheduler$FakeTaskTrackerManager.killJob(TestCapacityScheduler.java:359)
          	at org.apache.hadoop.mapred.CapacityTaskScheduler.killJobIfInvalidRequirements(CapacityTaskScheduler.java:1431)
          	at org.apache.hadoop.mapred.CapacityTaskScheduler.jobAdded(CapacityTaskScheduler.java:1463)
          	at org.apache.hadoop.mapred.JobQueuesManager.jobAdded(JobQueuesManager.java:183)
          	at org.apache.hadoop.mapred.TestCapacityScheduler$FakeTaskTrackerManager.submitJob(TestCapacityScheduler.java:387)
          	at org.apache.hadoop.mapred.TestCapacityScheduler.submitJob(TestCapacityScheduler.java:625)
          	at org.apache.hadoop.mapred.TestCapacityScheduler.testHighMemoryJobWithInvalidRequirements(TestCapacityScheduler.java:1992)
          
          Show
          Carlos Valiente added a comment - I don't think test failures are related to this patch: java.lang.NullPointerException at org.apache.hadoop.mapred.JobInProgress.terminateJob(JobInProgress.java:2117) at org.apache.hadoop.mapred.JobInProgress.terminate(JobInProgress.java:2153) at org.apache.hadoop.mapred.JobInProgress.kill(JobInProgress.java:2221) at org.apache.hadoop.mapred.TestCapacityScheduler$FakeTaskTrackerManager.killJob(TestCapacityScheduler.java:359) at org.apache.hadoop.mapred.TestCapacityScheduler.testClusterBlockingForLackOfMemory(TestCapacityScheduler.java:2069) and: java.lang.NullPointerException at org.apache.hadoop.mapred.JobInProgress.terminateJob(JobInProgress.java:2117) at org.apache.hadoop.mapred.JobInProgress.terminate(JobInProgress.java:2153) at org.apache.hadoop.mapred.JobInProgress.kill(JobInProgress.java:2221) at org.apache.hadoop.mapred.TestCapacityScheduler$FakeTaskTrackerManager.killJob(TestCapacityScheduler.java:359) at org.apache.hadoop.mapred.CapacityTaskScheduler.killJobIfInvalidRequirements(CapacityTaskScheduler.java:1431) at org.apache.hadoop.mapred.CapacityTaskScheduler.jobAdded(CapacityTaskScheduler.java:1463) at org.apache.hadoop.mapred.JobQueuesManager.jobAdded(JobQueuesManager.java:183) at org.apache.hadoop.mapred.TestCapacityScheduler$FakeTaskTrackerManager.submitJob(TestCapacityScheduler.java:387) at org.apache.hadoop.mapred.TestCapacityScheduler.submitJob(TestCapacityScheduler.java:625) at org.apache.hadoop.mapred.TestCapacityScheduler.testHighMemoryJobWithInvalidRequirements(TestCapacityScheduler.java:1992)
          Hide
          dhruba borthakur added a comment -

          Code looks good. +1.

          The test failures are not related to this patch.

          Show
          dhruba borthakur added a comment - Code looks good. +1. The test failures are not related to this patch.
          Hide
          dhruba borthakur added a comment -

          I have committed this patch. Thanks Carlos!

          Can some admin please re-assign this patch to Carlos?

          Show
          dhruba borthakur added a comment - I have committed this patch. Thanks Carlos! Can some admin please re-assign this patch to Carlos?
          Hide
          Hudson added a comment -

          Integrated in Hadoop-trunk #796 (See http://hudson.zones.apache.org/hudson/job/Hadoop-trunk/796/)
          . HDFS servers may start and stop external components through
          a plugin interface. (Carlos Valiente via dhruba)

          Show
          Hudson added a comment - Integrated in Hadoop-trunk #796 (See http://hudson.zones.apache.org/hudson/job/Hadoop-trunk/796/ ) . HDFS servers may start and stop external components through a plugin interface. (Carlos Valiente via dhruba)
          Chris Douglas made changes -
          Status Patch Available [ 10002 ] Resolved [ 5 ]
          Hadoop Flags [Reviewed]
          Assignee Carlos Valiente [ carlos.valiente ]
          Fix Version/s 0.21.0 [ 12313563 ]
          Resolution Fixed [ 1 ]
          Fredrik Hedberg made changes -
          Link This issue relates to HADOOP-5649 [ HADOOP-5649 ]
          Owen O'Malley made changes -
          Component/s dfs [ 12310710 ]
          Hide
          Robert Chansler added a comment -

          Editorial pass over all release notes prior to publication of 0.21.

          Show
          Robert Chansler added a comment - Editorial pass over all release notes prior to publication of 0.21.
          Robert Chansler made changes -
          Release Note Added support for HDFS pluggable components. New plugin facility for namenode and datanode instantiates classes named in new configuration properties dfs.datanode.plugins and dfs.namenode.plugins.
          Tom White made changes -
          Status Resolved [ 5 ] Closed [ 6 ]
          Brandon Li made changes -
          Link This issue is related to HADOOP-8832 [ HADOOP-8832 ]
          Matt Foley made changes -
          Link This issue is related to HDFS-3963 [ HDFS-3963 ]

            People

            • Assignee:
              Carlos Valiente
              Reporter:
              Carlos Valiente
            • Votes:
              1 Vote for this issue
              Watchers:
              13 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development