WFCORE-7077 SuspendController relies on blocking ServiceActivity behaviour for ordering #6267
Conversation
…vior for ordering
| } | ||
| } | ||
| } | ||
| } | ||
| }; | ||
| groupCompleter.accept(null, null); |
There was a problem hiding this comment.
Since groupCompleter.accept is being invoked here, wouldn't it be clearer if we get the rid of this BiConsumer and just transform it on a method that is directly invoked?
I mean, put everything inside groupCompleter.accept in its own method and avoid the usage of the BiConsumer
There was a problem hiding this comment.
Well, I tried simplifying it since it looks complex. I was just thinking whether it would be possible to split it into clearer chunks, my understanding is we are processing groups, and per each group, the activities in that group, so I was trying to at least get it split into those two methods to try to see it more clear, but it does not look simple.
If it is not possible, I have no complaints moving on with it, although looks complex and difficult to understand, at least to me :)
There was a problem hiding this comment.
Since groupCompleter.accept is being invoked here, wouldn't it be clearer if we get the rid of this BiConsumer and just transform it on a method that is directly invoked?
This BiConsumer is used as the completion handler per group.
groupStage.whenComplete(this);Rather than creating a new completion handler instance per group, each group shares the same instance.
There was a problem hiding this comment.
I expect that this looks complex because it uses recursion (i.e. the completion handler of each group stage registers itself as the completion handler of the next group stage).
I can replace this with a more verbose iterative implementation if that would be more intuitive.
There was a problem hiding this comment.
@pferraro feel free to ignore the above, now I got the issue and the resolution, in the old code we were moving to the next group even if the list of activities of the previous group has not been completed yet. Now this is fixed since the next group is not processed until all the activities have been completed.
| } | ||
| } | ||
| } | ||
| } | ||
| }; | ||
| groupCompleter.accept(null, null); |
There was a problem hiding this comment.
@pferraro feel free to ignore the above, now I got the issue and the resolution, in the old code we were moving to the next group even if the list of activities of the previous group has not been completed yet. Now this is fixed since the next group is not processed until all the activities have been completed.
| groupStage.complete(null); | ||
| } else { | ||
| // Counter used to determine when to complete group stage | ||
| AtomicInteger groupCounter = new AtomicInteger(activities.size()); |
There was a problem hiding this comment.
Nitpick: This should be named activityCounter instead groupCounter due to what it represents.
There was a problem hiding this comment.
Let me fix that - surely that's a potential source of confusion.
yersan
added
the
ready-for-merge
|
Thanks @pferraro |
https://issues.redhat.com/browse/WFCORE-7077