[Tables] insertMany ordered as true, middle faulty row should not fail the whole batch. #2193
Conversation
|
LGTM (I just tested from the perspective of a client, it's all good :) |
amorton
left a comment
amorton
left a comment
There was a problem hiding this comment.
We do not have stict ordering guarantees in the TaskGroup, and the predicate for finding the tasks before the current task can be stricter.
| /** | ||
| * Checks if, given the config of the container and the current state of the tasks, the container | ||
| * should fail fast and stop processing any further tasks. | ||
| * Determines whether the given {@code targetTask} should fail fast based on the container's |
There was a problem hiding this comment.
nit: replace container with TaskGroup
| * shouldFailFast(e) → true ('c' failed earlier, so 'e' must fail fast) | ||
| * </pre> | ||
| * | ||
| * @param targetTask the task to evaluate |
There was a problem hiding this comment.
can we clarify, should this be the next ask to be run or is it the last task that was run ?
| * <p>If sequential processing is enabled: | ||
| * | ||
| * <ul> | ||
| * <li>If the {@code targetTask} itself is in error state, it should fail fast. |
There was a problem hiding this comment.
this can happen if the task generated an error as it was being created, such as one insert in an insertMany with a unknown column name
| * </pre> | ||
| * | ||
| * @param targetTask the task to evaluate | ||
| * @return {@code true} if the container is configured for sequential processing and the {@code |
There was a problem hiding this comment.
nit: this part is implementation logic and can be removed "if the container is configured for sequential processing"
| return true; | ||
| } | ||
| // In sequential processing, if any prior task is in error state, we should fail fast | ||
| return stream() |
There was a problem hiding this comment.
because we are subclassing the array list it is mutatable AND there is no explicit ordering based on the position... it relies on the caller inserting the Tasks in position() order and no one else changing them.
It would be good to either 1) sort each time we iterate 2) stop subclassing and use something that supports custom order 3) stop subclassing, use an array list and sort on insertion 4) change the predicate as below.
But even if we change the predicate we still has not strictly ordered
| task -> { | ||
| var failFast = taskGroup.shouldFailFast(); | ||
| var failFast = taskGroup.shouldFailFast(task); | ||
| LOGGER.debug( |
There was a problem hiding this comment.
nit: can we move this to be DEBUG if failFast == true otherwise TRACE
| } | ||
| // In sequential processing, if any prior task is in error state, we should fail fast | ||
| return stream() | ||
| .takeWhile(task -> task != targetTask) |
There was a problem hiding this comment.
it may be safer and stricter that we want to get tasks that strictly have a position() that is before the position() of this task.
What this PR does:
When there is a faulty row in the entire batch of insertMany, and ordered as true.
Every row before the faulty row should succeed, and all the faulty row errors should propagate to the result.
Which issue(s) this PR fixes:
fixes: #1672
Checklist