[ https://issues.apache.org/jira/browse/MAPREDUCE-3535?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13167708#comment-13167708 ]
Steve Loughran commented on MAPREDUCE-3535: ------------------------------------------- Having a look at what I'd done w.r.t service lifecycle [http://svn.eu.apache.org/viewvc/hadoop/common/branches/HADOOP-3628-2/src/core/org/apache/hadoop/util/Service.java?view=markup] I avoided this by having the base class do all the checks in final methods and have overridable {{innerStart()}}, {{innerStop()}} etc. methods that subclasses would use to perform their custom operations, along with a static class to actually walk a service into its started state. There is no reason why the {{AbstractService}} class cannot use the same technique, without changing the {{Service}} interface. It would declare it's init/start/stop methods final, do the state change, then delegate to the {{protected}} methods {{innerInit()}} {{innerStart()}}, {{innerStop()}}. These would not be externally visible, and not get invoked until the validity of the operation had been tested. Effort: # time to rework the older service lifecycle methods into the new framework, 1 h # time to go through all the subclasses and rename their init/start/stop methods to be the inner ones. We could use a better prefix than "inner" if anyone can think of it. > Yarn service subclasses don't check for service state before executing their > state change operations > ---------------------------------------------------------------------------------------------------- > > Key: MAPREDUCE-3535 > URL: https://issues.apache.org/jira/browse/MAPREDUCE-3535 > Project: Hadoop Map/Reduce > Issue Type: Bug > Components: mrv2 > Affects Versions: 0.23.0, 0.24.0 > Reporter: Steve Loughran > > Although there are checks in the lifecycle state change methods (start, stop, > ...), they only get checked in the superclass. The subclasses don't check it; > they don't exit the start() method if they are already started, and they > don't bail out early on a stop if they are already stopped -even when the > superclass returns without doing anything. > This means that calling {{Service.start()}} twice may leak resources, > {{Service.start()}} twice try to shut down twice, etc. And that's on the same > thread... > My preferred action here would be for the ave the operations return true if a > state change took place, the implementation would be synchronised and return > the correct value > The subclasses look for this and only execute their state changes took place > e.g > {code} > boolean start() { > if (!super.start()) return false; > //do my own startup > return true; > } > {code} > The {{Service.stop()}} operation is trickier as the subclasses tend to call > the superclass last for a better unwinding. I think it may be safer to work > the other way around, but code reviews would be need to ensure that this > doesn't break assumptions in the subclass about termination order. It may be > possible to do more complex designs, but it is hard to chain this down a > hierarchy of classes. -- This message is automatically generated by JIRA. If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa For more information on JIRA, see: http://www.atlassian.com/software/jira