Hadoop Map/Reduce
  1. Hadoop Map/Reduce
  2. MAPREDUCE-2826

Change the job state observer classes to interfaces

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Incomplete
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None

      Description

      Schedulers will most often want to be the observers of the job state events in a single class. Therefore, I think they should be interfaces which can have multiple inheritance.

        Issue Links

          Activity

          Hide
          Doug Cutting added a comment -

          Why can't different observer implementations share state? This change might save a few lines of code, but at the price of breaking every implementation when an interface changes. That doesn't seem like a great tradeoff.

          Show
          Doug Cutting added a comment - Why can't different observer implementations share state? This change might save a few lines of code, but at the price of breaking every implementation when an interface changes. That doesn't seem like a great tradeoff.
          Hide
          Vivek Ratan added a comment -

          Given that JobInProgressListener has only abstract methods, it'll likely change because of additional/deletion/modification of abstract methods. That will break code anyway. Doug, where do you see JobInProgressListener as an abstract class being more amenable to change that it being an interface? Sure, an observer implementation can share state with the scheduler, but that comes with its own set of work - shared data structures have to be exposed, and hence wrapped in getters/setters or classes.

          So, given that the JobInProgressListener abstract class is just a collection of abstract methods, and likely will not contain any default state or behavior, I'm just not seeing how it's better to leave it that way than make it into an interface.

          Show
          Vivek Ratan added a comment - Given that JobInProgressListener has only abstract methods, it'll likely change because of additional/deletion/modification of abstract methods. That will break code anyway. Doug, where do you see JobInProgressListener as an abstract class being more amenable to change that it being an interface? Sure, an observer implementation can share state with the scheduler, but that comes with its own set of work - shared data structures have to be exposed, and hence wrapped in getters/setters or classes. So, given that the JobInProgressListener abstract class is just a collection of abstract methods, and likely will not contain any default state or behavior, I'm just not seeing how it's better to leave it that way than make it into an interface.
          Hide
          Doug Cutting added a comment -

          > [...] additional/deletion/modification of abstract methods [ ...] will break code anyway.

          In most cases, when an abstract API changes, one can emulate the old API with default implementations of the new methods. The old methods can be deprecated, to give warning that they will be removed in an upcoming release. For example, the FileSystem API has evolved substantially without breaking applications. Abstract methods have been added, deleted and modified without breaking applications or implementations.

          Show
          Doug Cutting added a comment - > [...] additional/deletion/modification of abstract methods [ ...] will break code anyway. In most cases, when an abstract API changes, one can emulate the old API with default implementations of the new methods. The old methods can be deprecated, to give warning that they will be removed in an upcoming release. For example, the FileSystem API has evolved substantially without breaking applications. Abstract methods have been added, deleted and modified without breaking applications or implementations.
          Hide
          Vivek Ratan added a comment -

          I agree with your comments in general, and it makes sense to look at an API as an abstract class first, for the reasons you've mentioned. However, in this particular case, it seems like we lose a lot by not having interfaces. It's not just a scheduler and a JobInProgressListener class that need to share state. We will most likely add support for listening to task changes. Once we have API for TaskProgressListener, it's very likely that the same class/data structures will be needed for handling job changes as well as task changes. Making TaskInProgressListener an abstract class will make this sharing more clunky.

          Furthermore, it's highly unlikely that we will have default implementations for JobInProgressListener and TaskInProgressListener. These really do expose abstract methods. I just can't see what any decent default implementations will do, without access to data structures that deal with JobInProgress objects and scheduling information.

          Show
          Vivek Ratan added a comment - I agree with your comments in general, and it makes sense to look at an API as an abstract class first, for the reasons you've mentioned. However, in this particular case, it seems like we lose a lot by not having interfaces. It's not just a scheduler and a JobInProgressListener class that need to share state. We will most likely add support for listening to task changes. Once we have API for TaskProgressListener, it's very likely that the same class/data structures will be needed for handling job changes as well as task changes. Making TaskInProgressListener an abstract class will make this sharing more clunky. Furthermore, it's highly unlikely that we will have default implementations for JobInProgressListener and TaskInProgressListener. These really do expose abstract methods. I just can't see what any decent default implementations will do, without access to data structures that deal with JobInProgress objects and scheduling information.
          Hide
          Steve Loughran added a comment -

          As someone who would add a listener to his todo list, can I state that it doesn't make that much difference to me. If it was an abstract class, I'd have to create a non-static inner class to handle the events; if it was an interface, I may still end up doing the same thing, though there it would be more optional.

          Show
          Steve Loughran added a comment - As someone who would add a listener to his todo list, can I state that it doesn't make that much difference to me. If it was an abstract class, I'd have to create a non-static inner class to handle the events; if it was an interface, I may still end up doing the same thing, though there it would be more optional.
          Hide
          Allen Wittenauer added a comment -

          I'm going to close this as stale.

          Show
          Allen Wittenauer added a comment - I'm going to close this as stale.

            People

            • Assignee:
              Vivek Ratan
              Reporter:
              Owen O'Malley
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development