THIS IS A TEST INSTANCE. ALL YOUR CHANGES WILL BE LOST!!!!
...
| Approach1 | Approach2 |
| Forces to execute operators even if prev operators failed. This can be a problem if subsequent operators after a failed operator throw exceptions other than dmlc::Error | Once there is a failed operator all the operators that depend on the current operator won't be executed. |
| Minimal api changes. | The lambda closure expected by PushAsync has a different signature after adding onstart callback. |
| Performance impact should be minimal in cases where there will be no exception thrown. | Performance impact needs to be investigated because of additional overhead of the onstart callback even for cases where no exception thrown. |
| For the cases where exception is thrown, there is an overhead of execution of subsequent operators. | For the cases where exception is thrown, there is no overhead of execution of subsequent operators. |
Recommendation
My recommendation is to take Approach1 since this introduces minimal api changes and also minimal performance impact in the case where no exception is thrown.
This will require a thorough examination of all operators to make sure they throw exceptions only wrapped in dmlc::Error.
The issue with Approach1 is that it forces execution of the following operators and may lead to incorrect results for variables that are directly or indirectly mutated. Also, the performance impact will be similar to Approach2 since we have additional overhead to propagate exceptions from the callback instead of onstart callback. The performance impact of adding Exception Handling Functionality needs to be investigated.
Open Questions
1. How to handle WaitForAll situation ?
...