optimize migrator's executeWriteFuncs

[debug-LiXiwen]

A Pull Request should be associated with an Issue.

• issue: 1123

Description

• Migrator's executeWriteFuncs will have a infinite loop when applyEventsQueue and copyRowsQueue is empty.
• So I Optimize the priority logic to avoid the infinite loop

[timvaillancourt]

This is an interesting idea. I left a few review comments, thanks @debug-LiXiwen! 🙏

[timvaillancourt]

@debug-LiXiwen thanks for the PR changes, it is passing CI now 👍

Now that the code looks good, I'm curious if you could explain what situations this optimizes?

If I understand correctly, this is the scenario this optimizes:

1. case eventStruct := <-this.applyEventsQueue() returns empty (no events)
2. Immediately after, events are added to the applier queue that were not seen by case in the last step
3. The select moves on to case copyRowsFunc := <-this.copyRowsQueue(), which is NOT empty
4. The new func this.drainApplierEventQueue() drains the events that appeared at step 2 (above)
   • On master today, the for loop would need to iterate again to catch the new applier events

Did I get this right and are there more scenarios?

The "window" for these events to appear above is extremely small. How likely is it that this scenario happens? Shouldn't the applier queue be empty by the time this.drainApplierEventQueue() is called? 🤔

[timvaillancourt]

One more requested change I noticed now

[debug-LiXiwen]

1. When the copyRowsQueue is not ready, you don't need to sleep manually, the select of the go language will be handled automatically
2. When the copyRowsQueue is ready, logically analyze the new way of processing to achieve better prioritization, but I did not verify the effect in the actual scene

[timvaillancourt]

1. you don't need to sleep manually, the select of the go language will be handled automatically

That's a good point, the time.Sleep() is a bit inefficient. Thanks for explaining 👍

[timvaillancourt]

cc @dm-2 / @rashiq for more 👀

[dm-2]

Thanks for your contribution @debug-LiXiwen! 🙇

1. When the copyRowsQueue is not ready, you don't need to sleep manually, the select of the go language will be handled automatically

I think relying on select to wait instead of using time.Sleep is a good idea as it makes the code easier to read. I don't think the performance difference will be significant, but I like that it makes the code flow better.

2. When the copyRowsQueue is ready, logically analyze the new way of processing to achieve better prioritization, but I did not verify the effect in the actual scene

I think drainApplierEventQueue() is unnecessary and we should remove it. I understand the reason for this optimisation, but the benefit will be very small and it makes the code more complicated than necessary. I prefer that we keep the code as simple as possible, and only optimise when there's a demonstrated need.

[timvaillancourt]

I don't think the performance difference will be significant, but I like that it makes the code flow better.

Agreed, I think it's very unlikely this would improve performance for most workloads

I think drainApplierEventQueue() is unnecessary and we should remove it

I would be on board with this 👍. It's is incredibly unlikely there are new events to apply immediately after the select already checked the apply queue