Pivot
  1. Pivot
  2. PIVOT-35

Replace Dispatcher with ExecutorService

    Details

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

      Activity

      Hide
      Todd Volkert added a comment -
      Show
      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
      Hide
      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
      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
      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
      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
      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
      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
      Greg Brown added a comment -

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

      Show
      Greg Brown added a comment - FYI, I have committed this change. Please let me know if you see any issues with it.
      Hide
      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
      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
      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
      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.

        People

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

          Dates

          • Created:
            Updated:
            Resolved:

            Development