Execute batch v2 Implementation#2427
Execute batch v2 Implementation#2427harshit-gangal wants to merge 17 commits intovitessio:masterfrom
Conversation
harshit-gangal
changed the title
harshit-gangal
changed the title
|
Currently this PR acts as model for batchParallel code changes. |
harshit-gangal
changed the title
|
@sougou PR up for review. |
|
@sougou - There is race condition in batch execute parallel code. Kindly check once. Review status: 0 of 43 files reviewed at latest revision, all discussions resolved, some commit checks failed. Comments from Reviewable |
sougou
left a comment
sougou
left a comment
There was a problem hiding this comment.
Changes look good overall. There's one suggested design change.
I'm still not fully comfortable with the query executor design, but can't think of a better alternative yet. We'll keep our minds open in that area for future improvements.
| @@ -0,0 +1,126 @@ | |||
| package engine | |||
There was a problem hiding this comment.
This sounded like a good idea when we discussed. But now I see that it makes everything awkward. I think the code will be much simpler if this was done in router.go. Just iterate, get plan & execute.
This approach will probably make sense if we decide to do the phases, but I think we may never do that if the parallel thing works out.
| }(index, plan) | ||
|
|
||
| } | ||
| go func() { |
There was a problem hiding this comment.
Found the bug: you need to wait for this function to also complete before returning.
Also, this function will currently never return because you're not closing ch anywhere.
|
The data race problem happens because we're now concurrently accessing the same topo info because of the parallelism. So, we need to improve the test framework to handle this. The non-sandbox code already allows concurrency for those calls. |
|
As per the discussion, this needs to be re-written. |
3 participants
This will be used as framework to support execute batch planning and execution phase.
This PR is dependent on #2423
This change is