fix(cluster): Wait before all access to slot migrations

[chakaz]

Fixes some regression test failures

[romange]

intuitively I find it not right that you wait for all the callbacks to finish in one of the callbacks. Moreover, seems from the first sight redundant since you already use AwaitFiberOnAll as a barrier.

[chakaz]

intuitively I find it not right that you wait for all the callbacks to finish in one of the callbacks

Why, though? That callback can yield to other callbacks if needed. It's not blocking on the shard queue, so what's wrong with that?
(further, one could argue that basically everything that runs in Dragonfly is callbacks :) )

Moreover, seems from the first sight redundant since you already use AwaitFiberOnAll as a barrier.

That's what I had in mind when I fixed this code originally (a few days ago). But the problem is that cancellation can happen in another thread (it comes from the client via the admin-port). So AwaitFiberOnAll wouldn't do on its own :(

[romange]

Callbacks can block, I am not saying there is a technical problem, but usually we do not synchronise callbacks between themselves when we run AwaitFiberOnAll - we wait for them to return all the information, aggregate it and then do the next step if needed. Maybe what's missing for me is to understand what the problematic use-case is.

[romange]

I think now I understand it actually. I would separate it into two
shard_set->pool()->Await(Fiber?)OnAll steps: the first one initializing the migration, and the second one doing the cancellable sync

[romange]

I think it's better to wrap 112-129 into block to avoid locking over AwaitFiberOnAll.

[romange]

LGTM