No recursion in dispatch error handling #1922
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1922 +/- ##
============================================
- Coverage 32.95% 32.94% -0.02%
+ Complexity 2656 2655 -1
============================================
Files 409 409
Lines 29257 29258 +1
Branches 3721 3721
============================================
- Hits 9643 9640 -3
- Misses 18731 18734 +3
- Partials 883 884 +1
Continue to review full report at Codecov.
|
juhoautio
force-pushed
the
no_recursive_dispatch_handling
branch
from
82c9a19
to
e7f8b3f
Compare
Pull Request Test Coverage Report for Build 4046
💛 - Coveralls |
juhoautio
force-pushed
the
no_recursive_dispatch_handling
branch
from
e7f8b3f
to
d7339da
Compare
|
@chengren311 thanks for checking in the unit tests. How about this one now? |
|
@chengren311 @kunkun-tang @HappyRay could you consider this, please? |
|
Or @jamiesjc ? |
| logger.error("Failed to dispatch queued execution " + exflow.getId() + " because " | ||
| + giveUpReason); | ||
| finalizeFlows(exflow); | ||
| // FAILED TOO MANY TIMES - exit |
There was a problem hiding this comment.
FAILED TOO MANY TIMES or no more executors?
There was a problem hiding this comment.
True that. I thought this comment kind of covers both.. but not as clearly as it could. I'd like to change the comment to: "GIVE UP DISPATCHING - exit". Ok?
| Set<Executor> remainingExecutors) { | ||
| if (reference.getNumErrors() >= this.maxDispatchingErrors) { | ||
| return "reached " + ConfigurationKeys.MAX_DISPATCHING_ERRORS_PERMITTED | ||
| + " (tried " + reference.getNumErrors() + " executors)"; |
There was a problem hiding this comment.
tried XX attempts instead of executors?
There was a problem hiding this comment.
"Executors" is correct currently, because each available executor is only tried once at most – if all have been tried, give up. Also, this loop is about dispatching a single execution.
juhoautio
force-pushed
the
no_recursive_dispatch_handling
branch
from
d7339da
to
21c596a
Compare
| logger.error("Failed to dispatch queued execution " + exflow.getId() + " because " | ||
| + giveUpReason); | ||
| finalizeFlows(exflow); | ||
| // GIVE UP DISPATCHING - exit |
There was a problem hiding this comment.
@chengren311 comment better now?
| selectedExecutor, exflow.getExecutionId()), e); | ||
| handleDispatchExceptionCase(reference, exflow, selectedExecutor, | ||
| availableExecutors); | ||
| while (true) { |
There was a problem hiding this comment.
can we use for loop here? my concern for while(true) is it's exposed to the risk of infinite loop if some exit condition is not well defined somehow.
There was a problem hiding this comment.
@chengren311 I changed while loop to for.
The code had a recursion loop like this:
selectExecutorAndDispatchFlow
-> handleDispatchExceptionCase
-> selectExecutorAndDispatchFlow
-> ..
..with max depth of activeExecutors.size()
Iterative approach is easier to reason about, and is totally safe against StackOverflowError.
This is also preliminary refactoring to eventually allow configuring higher max for dispatching errors. Currently, however, max dispatch attempts is capped by the total number of active executors (because each executor is only tried once). In the future I'd like to remove that limit and rely solely on the MAX_DISPATCHING_ERRORS_PERMITTED property.
juhoautio
force-pushed
the
no_recursive_dispatch_handling
branch
from
21c596a
to
cd23416
Compare
|
thanks! @juhoautio |
|
Thank you!! |
The code had a recursion loop like this:
..with max depth of
activeExecutors.size()Iterative approach is easier to reason about, and is totally safe against
StackOverflowError.This is also preliminary refactoring to eventually allow configuring higher max for dispatching errors. Currently max dispatch attempts are capped by the total number of active executors (because each executor is only tried once at most). In the future I'd like to remove that limit and rely solely on the
MAX_DISPATCHING_ERRORS_PERMITTEDproperty.