Uploaded image for project: 'Pivot'
  1. Pivot
  2. PIVOT-35

Replace Dispatcher with ExecutorService

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.5
    • Component/s: core-util
    • Labels:

      Activity

      Hide
      gbrown Greg Brown added a comment -

      I think the existing implementation is preferable. In general, we try to avoid delegating constructor code to an init() method, since that is what a constructor is for. Also, there is no harm in making the default ExecutorService public and available to Task subclasses.

      Show
      gbrown Greg Brown added a comment - I think the existing implementation is preferable. In general, we try to avoid delegating constructor code to an init() method, since that is what a constructor is for. Also, there is no harm in making the default ExecutorService public and available to Task subclasses.
      Hide
      dmoebius Dirk Moebius added a comment -

      Hi Greg, your change looks very good. I thought of getting rid of Dispatcher, too, but then I thought that this change might be to big.

      A minor nitpick: I'd change the Task subclasses such as LoadTask or Query to use the no-arg super constructor instead of using the constructor with ExecutorService parameter, so that the subclasses don't need to be aware of DEFAULT_EXECUTOR_SERVICE at all. My attached patch changes this.

      Show
      dmoebius Dirk Moebius added a comment - Hi Greg, your change looks very good. I thought of getting rid of Dispatcher, too, but then I thought that this change might be to big. A minor nitpick: I'd change the Task subclasses such as LoadTask or Query to use the no-arg super constructor instead of using the constructor with ExecutorService parameter, so that the subclasses don't need to be aware of DEFAULT_EXECUTOR_SERVICE at all. My attached patch changes this.
      Hide
      gbrown Greg Brown added a comment -

      FYI, I have committed this change. Please let me know if you see any issues with it.

      Show
      gbrown Greg Brown added a comment - FYI, I have committed this change. Please let me know if you see any issues with it.
      Hide
      gbrown Greg Brown added a comment -

      See attached patch. I think this approach is preferable to wrapping the ExecutorService in a Dispatcher, since it requires less code and is more flexible.

      Thanks for the suggestion to use ExecutorService.

      Show
      gbrown Greg Brown added a comment - See attached patch. I think this approach is preferable to wrapping the ExecutorService in a Dispatcher, since it requires less code and is more flexible. Thanks for the suggestion to use ExecutorService.
      Hide
      gbrown Greg Brown added a comment -

      I like this patch. But might it be better to simply modify Task to use ExecutorService directly? There's not much left in Dispatcher at this point.

      Show
      gbrown Greg Brown added a comment - I like this patch. But might it be better to simply modify Task to use ExecutorService directly? There's not much left in Dispatcher at this point.
      Hide
      dmoebius Dirk Moebius added a comment -

      This patch changes Dispatcher to use java.util.ExecutorService. This because of two reasons:

      1. ExecutorService implementations provide all of that functionality that is missing and marked with TODO in the current Dispatcher implementation. In fact, my patch removes all TODOs in Dispatcher, because all of them have been dealt with.
      2. Implementing a correctly working thread pool is a hard task. No offense, but I trust the Java core developers more than you when it comes to multi thread programming. Not because I think they're better programmers (probably not), but because they have a LOT of tests running up and down all the time, and there are more eyes watching. Millions.

      The patch reduces Dispatcher to a thin facade over ExecutorService. Note that since I added a shutdown() method (you get this one for free), there's now a way to let ApplicationContext control the dispatcher lifecycle.

      For motivation of this patch see discussion here: http://mail-archives.apache.org/mod_mbox/pivot-user/201004.mbox/%3CA820E617-DC9A-4843-89E4-36320482A1CF@mac.com%3E

      Show
      dmoebius Dirk Moebius added a comment - This patch changes Dispatcher to use java.util.ExecutorService. This because of two reasons: 1. ExecutorService implementations provide all of that functionality that is missing and marked with TODO in the current Dispatcher implementation. In fact, my patch removes all TODOs in Dispatcher, because all of them have been dealt with. 2. Implementing a correctly working thread pool is a hard task. No offense, but I trust the Java core developers more than you when it comes to multi thread programming. Not because I think they're better programmers (probably not), but because they have a LOT of tests running up and down all the time, and there are more eyes watching. Millions. The patch reduces Dispatcher to a thin facade over ExecutorService. Note that since I added a shutdown() method (you get this one for free), there's now a way to let ApplicationContext control the dispatcher lifecycle. For motivation of this patch see discussion here: http://mail-archives.apache.org/mod_mbox/pivot-user/201004.mbox/%3CA820E617-DC9A-4843-89E4-36320482A1CF@mac.com%3E
      Hide
      tvolkert Todd Volkert added a comment -
      Show
      tvolkert Todd Volkert added a comment - This ticket serves as a container for all TODO tasks in http://svn.apache.org/repos/asf/incubator/pivot/trunk/core/src/org/apache/pivot/util/concurrent/Dispatcher.java

        People

        • Assignee:
          gbrown Greg Brown
          Reporter:
          gbrown Greg Brown
        • Votes:
          0 Vote for this issue
          Watchers:
          0 Start watching this issue

          Dates

          • Created:
            Updated:
            Resolved:

            Development