On 1 August 2011 19:39, Greg Brown <[email protected]> wrote:
>>> Actually, it just occurred to me that uniqueness, while not strictly
>>> required, should certainly be encouraged. If you added the same task to a
>>> ParallelTaskGroup's task sequence more than once, you'd basically get
>>> serialized behavior, since only one thread could run the task at a time. So
>>> perhaps a Group is actually appropriate.
>>
>> I think that really depends on the intent again. I agree that in many
>> cases the supplied Tasks would not contain any duplicates, but is
>> there a strong reason to actually enforce it rather than just document
>> the behaviour?
>
> I think the strongest argument for Group is that allowing duplicates could
> result in confusing behavior. Making it a Group ensures that the intent is
> clear.
Making it a Group certainly helps with the intent, but wouldn't
eliminate confusing behaviour entirely as my example of the
'SingleThreadExecutor' suggests. Better that just accepting a
Collection / Iterable / Array or whatever.
A few words in the documentation could make the intent explicit.
>>> Worse, TaskSequence executes sub-tasks by creating (or at least obtaining)
>>> a new thread per task, which isn't really necessary. The same results could
>>> be achieved by simply calling the synchronous version of execute() (the one
>>> that doesn't take a listener argument) on each task in the list.
>>
>> This was part of the cause of my confusion with these classes. I
>> didn't see why they needed to be Tasks themselves. It is
>> straightforward to create and execute new Task if and when I want to,
>> but I can't extract the executable portion of code from a Task once it
>> is created. If the parallel execution code is wrapped in a Task, then
>> that decision has been made for me.
>
> That's true, and perhaps it is reasonable to consider making TaskGroup a
> non-Task class. OTOH, if such a class is not wrapped in a Task, the calling
> thread will block while it waits for the sub-tasks to complete, which may not
> be the desired behavior. If the 90% case is that a TaskGroup is run in its
> own Task, then maybe it should actually be a Task.
I don't really feel strongly either way now that I better understand
the intent of the classes. As you say, no point forcing someone to
create a task just to wrap another call if that is the most common
usage pattern. Another option would be a utility method to create and
execute the wrapper Task.
>> Do you remember if it was a conscious choice for these classes to
>> extend Task from the outset, or do they do so because they use
>> TaskListener for notifications?
>
> I think they just seemed like a natural fit for a Task - TaskGroup allows a
> caller to create a task hierarchy in the same way that Component and
> Container create a UI hierarchy.
I can understand why you went that way, and don't have a problem with
it. My brain was thinking about another approach and got confused my
trying to make the existing approach fit my (mis)conceptions.
> The idea of using a static execute() method to launch parallel tasks is
> interesting:
>
> public class TaskGroup {
> public static void execute(Sequence<Task> tasks, ExecutorService
> executorService) { ... }
> public static void execute(Sequence<Task> tasks, TaskListener listener,
> ExecutorService executorService) { ... }
> }
>
> If I understand your suggestion correctly, the first version would basically
> do what TaskGroup#execute() does now. The second version would wrap a call to
> the first version in a Task (probably implemented as a static inner class)
> and basically do what TaskGroup#execute(TaskListener, ExecutorService) does
> now. Is that correct?
>
> If so, I could go either way. I'm not sure which would be clearer to most
> developers - a dedicated TaskGroup class that extends Task, or these static
> methods. I'm inclined to think the former, but maybe that is just personal
> preference.
>
> G
Yeah the first is just the synchronous call, and the second makes the
wrapper task for you.
I kept playing around with the idea of distilling everything into some
static utility methods and ended up with something that seems to work
quite well. I included a very quickly put together 'fluent' style
'builder' so that the following code is legal.
// Sync
Tasks.executeThese(tasks).synchronously();
// Async
final ExecutorService threadPool = Executors.newFixedThreadPool(3);
final ShutdownExecutorTaskListener executorShutdownListener = new
ShutdownExecutorTaskListener(threadPool);
Task<Void> task = Tasks.executeThese(tasks)
.overridingExecutorsWith(threadPool)
.executeParentTaskWith(parentTaskExecutorService)
.notifying(new SysOutTaskListener<Object>(),
executorShutdownListener)
.asynchronously();
The entry points for the 'fluent' API are these 2 methods
public static TaskExecutionHelper executeThese(final Task<?>... tasks) { ... }
public static TaskExecutionHelper executeThese(final Iterable<?
extends Task<?>> tasks) { ... }
The 'fluent' API just calls the appropriate overloaded
'executeAsynchronously' or 'executeSynchronously' static methods, so
isn't mandatory
I can post it if you want to see the full thing (not much testing yet
- it it was intended to just be a proof of concept, but then got very
well fleshed out)
Chris
