Export namenode/datanode functionality through a pluggable RPC layer

• Key: HADOOP-5257
• Type: New Feature
• Status: Resolved
• Resolution: Fixed
• Priority: Minor
• Assignee: Carlos Valiente
• Reporter: Carlos Valiente
• Fix Version/s: 0.21.0

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

Comments

[Doug Cutting]

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.

[dhruba borthakur]

@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.

[Doug Cutting]

> 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.

[Carlos Valiente]

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]

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

[dhruba borthakur]

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.

[Carlos Valiente]

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();
}

[Konstantin Shvachko]

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.

[Carlos Valiente]

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]

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

[dhruba borthakur]

> 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.

[Carlos Valiente]

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.

[Konstantin Shvachko]

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.

[Carlos Valiente]

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?

[Doug Cutting]

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.

[Carlos Valiente]

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]

Minor fixes for 'ant checkstyle'

[Steve Loughran]

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

[Carlos Valiente]

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

[Hadoop QA]

+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.

[dhruba borthakur]

+1 for this code.

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

[Steve Loughran]

@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.

[dhruba borthakur]

> 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?

[Carlos Valiente]

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.

[Steve Loughran]

@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

[Steve Loughran]

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.

[Carlos Valiente]

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?

[Carlos Valiente]

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

[dhruba borthakur]

Cancelling patch to re-trigger hudsonqa tests

[dhruba borthakur]

Trigger hadoopQA tests

[Hadoop QA]

-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.

[Carlos Valiente]

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)

[dhruba borthakur]

Code looks good. +1.

The test failures are not related to this patch.

[dhruba borthakur]

I have committed this patch. Thanks Carlos!

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

[Hudson]

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)

[Robert Chansler]

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