Here's an updated patch with fixes and new test.
Rohith Sharma K S
One suggestion is can default value for threshold reduce to less than 50%?
1) Should we disable am-blacklisting by default?
I would like to tackle both of these as part of
YARN-4685 so that others also can see.
2. I am little bit confused with naming convention for blacklist with placesBlacklist. And is there any plan to support blacklist racks in the future?
Yes, that's the idea. In other parts of the code, this list gets passed along to filter both nodes and racks.
Regarding renames, I've included the ones you pointed out. There are lot more to be done, but I deliberately avoided them given the current size of the patch.
Addressed other comments.
6) ResourceBlacklistRequest -> (Resource)Place(ment)BlacklistRequest?
This is public API, cannot rename it now.
Created a new TestNodeBlacklistingOnAMFailures, moved existing tests from TestAMRestart to this new class file. testAMBlacklistPreventsRestartOnSameNodeForMinicluster() is a bogus test, removed it.
1. yarn.resourcemanager.am-scheduling.node-blacklisting-enabled and yarn.resourcemanager.am-scheduling.node-blacklisting-disable-threshold to be added in yarn-default.xml.
Again, I deliberately deleted them for now. I'd like to discuss their re-addition as part of the outcome for