Add Tasks.seqActions(...) to sequence actions #17

Open
wants to merge 1 commit into
from

Projects

None yet

5 participants

@cpettitt
LinkedIn member

@cheftako @sweeboonlim @chikit @brikis98

This new task makes it easy to sequence a set of actions when you don't
need the result from the last task. Our current Tasks.seq(...) tasks
return the value of the last task. When recursively sequencing fan out
(parallel tasks) this can lead to a List...>> type for
the seq task when subsequent aggregation does not occur.

The behavior of seqActions differs from seq in that any task that fails
in the sequence causes the sequence to terminate immediately with an
error.

@cpettitt cpettitt Add Tasks.seqActions(...) to sequence actions
This new task makes it easy to sequence a set of actions when you don't
need the result from the last task. Our current Tasks.seq(...) tasks
return the value of the last task. When recursively sequencing fan out
(parallel tasks) this can lead to a List<List<...List<T>...>> type for
the seq task when subsequent aggregation does not occur.

The behavior of seqActions differs from seq in that any task that fails
in the sequence causes the sequence to terminate immediately with an
error.
c5f76c3
@chikit chikit commented on the diff Oct 12, 2012
src/com/linkedin/parseq/SeqActionsTask.java
+ {
+ @Override
+ public void onResolved(final Promise<Object> promise)
+ {
+ if (promise.isFailed())
+ {
+ if (count.get() > 0) {
+ count.set(0);
+ result.fail(promise.getError());
+ }
+ }
+ else
+ {
+ if (count.decrementAndGet() == 0)
+ {
+ result.done(null);
@chikit
chikit Oct 12, 2012

Is it possible to have a count of 1 and enters the if(count.get()>0) then context switch to another thread and decrementAndGet() ==0 which would cause a race condition on result.fail and result.done?

@sweeboonlim
sweeboonlim Oct 12, 2012

This depends on the execution order guarantees provided by the framework.
If the next task cannot execute before the call back on onResolved has returned on the earlier task, then there is no concurrency problem. I think this is a reasonable guarantee since the next task may depend on the side effects from of onResolved.

@cpettitt
cpettitt Oct 12, 2012

As Swee points out, it's not possible for a race for tasks executed by the engine. However, there is one way we can have a race: direct cancellation of the task by the user. Ultimately that cancellation should probably go through the engine. For now, we should give this code proper treatment for concurrency.

@brikis98 brikis98 commented on the diff Oct 12, 2012
test/com/linkedin/parseq/TestSeqActionsTask.java
+import static org.testng.AssertJUnit.assertNull;
+import static org.testng.AssertJUnit.assertTrue;
+import static org.testng.AssertJUnit.fail;
+
+/**
+ * @author Chris Pettitt
+ */
+public class TestSeqActionsTask extends BaseEngineTest
+{
+ @Test
+ public void testWithEmptyList()
+ {
+ try
+ {
+ seqActions();
+ fail("Should have thrown IllegalArgumentException");
@brikis98
brikis98 Oct 12, 2012

TestNg supports "expected exceptions":

@Test(expectedExceptions = IllegalArgumentException.class)
public void testWithEmptyList() { ... }
@brikis98 brikis98 commented on the diff Oct 12, 2012
test/com/linkedin/parseq/TestSeqActionsTask.java
+ }
+ catch (IllegalArgumentException e)
+ {
+ // Expected case
+ }
+ }
+
+ @Test
+ public void testWithSequenceOfActions() throws InterruptedException
+ {
+ final int numTasks = 10;
+ final AtomicInteger counter = new AtomicInteger();
+ final List<Task<?>> tasks = new ArrayList<Task<?>>();
+ for (int i = 0; i < numTasks; ++i)
+ {
+ final int finalI = i;
@brikis98
brikis98 Oct 12, 2012

lol. I heart Java.

@brikis98

I'm not sure the name SeqActionsTask adequately captures how this differs from the normal seq. Unfortunately, I can't come up with a good name. Best I've got so far is SideEffectShortCircuitSeqTask...

@sweeboonlim sweeboonlim commented on the diff Oct 12, 2012
src/com/linkedin/parseq/Tasks.java
@@ -197,6 +197,30 @@ private Tasks() {}
}
/**
+ * Creates a new task that will run each of the supplied tasks in order. If
@sweeboonlim
sweeboonlim Oct 12, 2012

Please add comment to seq indicating (1) failure of a task does not cause subsequent tasks not to be executed, (2) a link to seqAction that does fail early if a task fails.

I suggest the following other names for this:

seqFailFast
seqFailStop
seqStopOnFailure

@gzak-li
gzak-li Aug 29, 2013

Although it's probably too late now, I would actually have suggested the other way around: this new method should be called seq() and the existing one should be called seqContinueOnFailure() or something to that effect, as I'm pretty sure most users expect the fail-fast behavior when executing sequences to be the default (cursory poll of some colleagues confirms this suspicion).

Think of it this way: using seq() pretty much automatically means there's some kind of dependency between the tasks, otherwise why not just use par() instead?

@cpettitt
LinkedIn member

I think I muddied the water with this commit by doing semi-unrelated things in the new task:

  1. Dropping the result value and returning null (original purpose of the commit)
  2. Failing fast when a task in the sequence fails

I grouped them because it seems that firing off a sequence of actions and ignoring the final result implies the user just wants to make a sequence of side effects occur. Without fail fast they will need to check each task in the sequence to ensure that the sequence succeeded (or we can still propagate the error, but as the last step).

What if I make failFast an optional parameter to seqActions and when !failFast we still mark the sequence failed at the end? It would seem that in this case the default of failFast == true is safer. Does this make things any clearer? Alternative proposals are welcomed.

@brikis98

failFast sounds like a useful feature to be able to control for all seq commands, side-effect driven or otherwise. It's not necessary to include it in this checkin, but it's worth thinking about having as a general feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment