Force Refresh Listeners when Acquiring all Operation Permits

[original-brownbear]

• Fixes the issue reproduced in the added tests:
  • When having open index requests on a shard that are waiting for a refresh, relocating that shard
    becomes blocked until that refresh happens (which could be never as in the test scenario).
• Fixed by:
  • Before trying to aquire all permits for relocation, refresh if there are outstanding operations

---

PS: I ran the added tests for a few thousand runs without trouble.

[elasticmachine]

Pinging @elastic/es-distributed

[original-brownbear]

It seems the only production use case for this method is in relocation so admit it's a little noisy to add this kind of general callback here, but it still seems like the smallest possible change to get a hook to run the refresh conditionally here (after preventing new operations from piling on more waits concurrently).

[ywelsch]

This has still a race I think. The issue is that there's no guarantee that the refresh will happen after all pending requests have registered a refresh listener. To ensure this, we need multiple steps. First, ensure that no new listeners are registered (this can be achieved by setting getMaxRefreshListeners to 0), and then doing a manual refresh to free all existing listeners. There is no need I think to inline all this into blockOperations, it can be done before calling this method.

[original-brownbear]

@ywelsch alright, yours is a much better plan :) => reverted my approach and implemented that in f40c6a6 (sorry for accidental rebase)

[ywelsch]

Thanks @original-brownbear. The concurrency looks better. I think we need to extend this to all actions that can possibly acquire all operation permits. In particular, I think this might also cause problems on replicas, e.g. when a replica learns of a new primary and tries to bump its term (see IndexShard#bumpPrimaryTerm). If it then has a refresh=wait_for op waiting (from the old primary), it will run into the same issue, and indefinitely stop accepting any writes from the new primary.

[ywelsch]

As we always want to do the refresh after calling disallowAdd, I wonder if we should combine both into one method

[original-brownbear]

I couldn't find a neat way of doing that since we have the async case and the blocking case of acquiring all the permits and we want to enforce some try-finally semantics for allowing the listeners again for both now. I'm not sure it actually makes things more readable if we hide the handling of exceptions from refresh(...) in some other method. I can try finding a nice way though :)

[original-brownbear]

@ywelsch all points addressed I think => should be good for another review.

I think we need to extend this to all actions that can possibly acquire all operation permits.

Done, I wrapped all cases of acquiring all permits I could find. It seems though, that IndexShard#bumpPrimaryTerm was the only production code use-case.
The other two methods where I added the logic seem to only be called from tests.

[ywelsch]

I've pushed
daa9fc7 which simplifies the code imho. We will also need unit tests for the RefreshListeners class.

[original-brownbear]

Ok thanks, I'll add some tests :)

[original-brownbear]

@ywelsch ok fixed :) Added test in 71bef56 Should be good for review now

[ywelsch]

LGTM

[ywelsch]

maybe we could assert before this line here that assert refreshListeners == null?

[ywelsch]

Please adapt PR title to make it clear this is not only for relocations. For example: "Force refresh listeners when acquiring all operation permits"

[tlrx]

Thanks for fixing this @original-brownbear !