Bibin A Chundatt Thanks for reporting. I see it works fine for me.

Does it happen every time for you, or any specific case it happens?

Thanks Devaraj K for checking the issue

All the time when i start the server first 2 nodemanagers and then Resource manager.

As mentioned in defect describtion when i start Resourcemanager exclude file was configured with NM1 host name and both Node Manager 1 and 2 were up .

Then resource mangers starts up is does send signal also

2015-01-30 17:15:11,549 INFO org.apache.hadoop.yarn.server.resourcemanager.ResourceTrackerService: Disallowed NodeManager from HOST-10-19-92-128, Sending SHUTDOWN signal to the NodeManager.
But table is shown as empty

I met the same problem. Hi, Naganarasimha G R, can I take this issue?

Hi Hong Zhiguo, I had actually started to work on this patch but was skeptical that YARN-914 (or its subjira's ) might have impact or take care of this issue. Give me couple of days time, will try the check the state of my patch and update you.

Following the discussion from YARN-4402:

Given that we consider exclude list as canonical truth of decomm-ed nodes, which means during serviceInit, setDecomissionedNMsMetrics call is kept as is, the only way to have these nodes be part of inactiveNodes map, which today gets reinitialized to a new empty concurrent map in RMActiveServiceContext during startup, is to have node hostnames/ips read in from exclude list and added to this map even though we lose the port information. This is because the node would ideally not have the NM process running and we don't keep that state across RM restarts. What that means is, we add a (NodeId,RMNode) entry where the hostname is legal but the ports are a defined invalid value like -1. This allows us to track the nodes that were decommissioned in the previous life cycle of the RM. We can also tweak the GUI to display N/A when the port is -1. Since the check of isValidNode is only on the basis of hostname/ip , this does not affect the rejoining behavior of the node. Requesting Eric Payne for comments and ideas.

Sounds like a generally reasonable approach. I definitely agree that the exclude list will need to be read on the host files refresh.

This patch adds semi-dummy <NodeID,RMNode> entries to active RMNodes map on init() for all nodes in the exclude list. Such a node is then transitioned right away to DECOMMISSIONED state from NEW state. Also, to fix the metric discrepancy if such a node joins back, the AddNodeTransition is modified. A simple test case is included.

Another approach can instead add the functionality of making transitions on inactiveRMNodes as well. This would allow not adding such dummy nodes to active list in the first place and put it in the inactive list instead.

-1 overall

This message was automatically generated.

Updating patch after addressing findbugs warning, failed test case and check-style issues. Javadocs issue seems unrelated and could be connected to YARN-4567.

Test failures in TestRMRestart were due to missing wait before checking the metrics since now we transition the decommissioned RMNodes candidates instead of just populating the metric count based on exclude list.

Test failure in TestFifoScheduler was not locally reproducible.

-1 overall

This message was automatically generated.

Test failures are from known issues mentioned earlier.

Requesting Daniel Templeton, Jason Lowe, Eric Payne for comments and review.

Thanks, Kuhu Shukla.

I'd really rather see the anonymous inner classes used for the dummy nodes be named inner classes. It would make the code easier to read.

In the unit tests, I'd also like to see you test the same two scenarios when the includes list is non-empty, plus what happens when the excludes list contains nodes that aren't registered, plus what happens when the includes and excludes list overlaps.

I agree with Daniel's comments.

Rather than manually scanning the inactive nodes list, can we build an inactive node instance using the registering node's host and try to remove it from the set? Should be a lot quicker as the number of inactive nodes gets large.

For the tests, I really would like to avoid sleeps whenever possible. We either are sleeping too long leading to wasted time during tests (Hadoop test runs are far too long as it is) or we don't wait enough and the test fails. They often lead to flaky tests as hosts get loaded during test runs, etc. We're already using the drain dispatcher, can't we simply wait for the dispatcher events to drain rather than rely on magical sleep times?

Thank you very much Daniel Templeton and Jason Lowe for the review. I have updated the patch accordingly. There is an additional test case with one node that is in the exclude list but does not register. It is accounted for, during RM startup as it is present in the list but does not impact metrics during refreshNodes since we look at RMNode instances of only active nodes which this host is not.

While updating the test for YARN-3102, I modified the writeToHostsFile to have the file object passed as an argument. I am updating this patch with the same change to be consistent. No changes to the non-test code.

Please ignore, updated the wrong JIRA.

-1 overall

This message was automatically generated.

-1 overall

This message was automatically generated.

Corrected minor checkstyle warnings. Test failures are systemic as mentioned previously.

-1 overall

This message was automatically generated.

Addressed findbugs warning.

-1 overall

This message was automatically generated.

Daniel Templeton, Jason Lowe, Requesting for review comments. Thanks a lot!

Thanks, Kuhu Shukla! Looks pretty good. Last thing I have to pick at is the dummy classes. Can you add some javadocs for DummyNode? I also think that DummyId and DummyNode are not great names. What about something more descriptive like UnknownId and UnknownHost?

Thanks a lot Daniel Templeton. Updated the patch.

Looks good to me. Jason Lowe?

-1 overall

This message was automatically generated.

Test failures are unrelated and tracked elsewhere.

Patch looks pretty good, but there are some nits:

Not caused by this JIRA, but the node rejoin transition has the contains-followed-by-get problem. If another thread removes the node after the containsKey call then the get will return null but the code doesn't handle that and will NPE. It is safer and faster to just do the get then check for null rather than perform the key lookup twice. This also avoids the awkward declaring of the previousRMNode variable with no initial value.

Similarly we should not call get-then-remove when checking for the unknown node. Instead we can simply call remove on the unknown node ID and check if the remove returned anything to know if it was there.

In TestResourceTrackerService, wouldn't it be simpler to create an overloaded form of writeToHostsFile that takes the specified file rather than replacing the existing method? Then we can implement the original method in terms of the new method and cut out a large portion of this patch where it has to fixup all the original calls of the removed method.

4 seconds seems pretty aggressive for a test timeout, especially with multiple RM bringups and teardowns involved. If this runs on a sluggish jenkins machine that happens to pause at the wrong time then the test fails – i.e.: is it important that this test fails if it executes in 5 seconds instead? Seems like the timeout should be at least 20 seconds, if there is an explicit timeout specified at all (surefire has one built in).

Should we just change MockRM to use the drain dispatcher and expose a drain events method rather than fix a bunch of places to override which dispatcher to use?

Thanks a lot Jason Lowe for the very useful comments and help. I have updated the patch to have a default DrainDispatcher for MockRM. Also reduced the containsKey-get-remove to a remove and get-remove to remove in RMNodeImpl. I fixed 2 test classes where the drainDispatcher was following older code style along with the writetoHostFile change. Test cases' timeouts are now 30 seconds.

-1 overall

This message was automatically generated.

Jason Lowe, Request for comments/review. Thank you!
CC: Daniel Templeton

Shouldn't this get be a remove? Otherwise I think we leave the old node in the inactive list.

      RMNode previousRMNode = rmNode.context.getInactiveRMNodes().get(nodeId);
      if (previousRMNode != null) {
        rmNode.updateMetricsForRejoinedNode(previousRMNode.getState());
      } else {

Why is MockRM#drainEvents a protected function instead of public? Seems like this would make it difficult to use from other java packages without needlessly deriving from it.

Nit: The new writeHostsFile method uses a local variable that eclipses the existing hostFile instance variable. It would be a bit clearer if that aliasing wasn't there.

My bad. Updated the patch. Thank you Jason Lowe.

-1 overall

This message was automatically generated.

Test failures are unrelated. Jason Lowe, request for review/comments. Thanks again!

Thanks, Kuhu! Latest patch looks good to me. It doesn't apply cleanly to branch-2.7, could you provide a patch for that as well?

Uploading branch-2.7 version of the patch. The issue mainly in RMNodeImpl since InactiveRMNodes is a Map of <String, RMNode> and not <NodeId,RMNode>. where the mapping is between the hostname (not including the port) and the RMNode. When the AddTransition tries to remove the entry for the host it checks additionally based on port if the result is not null.

-1 overall

This message was automatically generated.

+1, the branch-2.7 patch lgtm. Not sure what happened to the precommit build. I applied the patch to branch-2.7 and all the resourcemanager tests passed.

Committing this.

Thanks to Kuhu for the contribution and to Daniel for additional review! I committed this to trunk, branch-2, branch-2.8, and branch-2.7.

FAILURE: Integrated in Hadoop-trunk-Commit #9219 (See https://builds.apache.org/job/Hadoop-trunk-Commit/9219/)
YARN-3102. Decommisioned Nodes not listed in Web UI. Contributed by Kuhu (jlowe: rev ed55950164a66e08fa34e30dba1030c5a986d1f1)

• hadoop-yarn-project/CHANGES.txt
• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/rmnode/RMNodeImpl.java
• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/NodesListManager.java
• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/MockRM.java
• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/ResourceManager.java
• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/TestRMRestart.java
• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/TestResourceTrackerService.java

Closing the JIRA as part of 2.7.3 release.