Inactive shard flush should wait for ongoing one

[kingherc]

org.elasticsearch.indices.flush.FlushIT#testFlushOnInactive would
sometimes fail in the following case:

• SHARD_MEMORY_INTERVAL_TIME_SETTING is set very low, e.g., 10ms
• The regularly scheduled multiple flushes proceed to
  org.elasticsearch.index.shard.IndexShard#flushOnIdle
• There, the first flush will handle e.g., the first document
  that was indexed. The second flush will arrive shortly after,
  before the first flush finishes.
• The second flush will find that wasActive = true (due to the
  indexing of the remaining documents), and will set it to false.
• However, the second flush will not be executed because
  waitIfOngoing = false, and there is the ongoing first flush.
• No other flush is scheduled (since any next regularly scheduled
  flush will find wasActive = false), which creates the problem.

Fixes #87888

[elasticsearchmachine]

Pinging @elastic/es-distributed (Team:Distributed)

[kingherc]

Hi @henningandersen and @DaveCTurner . After investigating the test failure, we (with the help of @fcofdez ) figured out the situation, which is presented in the PR description. It can happen in some rare situations when the SHARD_MEMORY_INTERVAL_TIME_SETTING is set too low and/or if a flush takes too long.

There are 3 solutions we discussed so far:

1. The current PR's solution, to change the waitIfOngoing to true. Drawback: one of the flush threadpool's threads may wait on the flushLock. But for normal values of SHARD_MEMORY_INTERVAL_TIME_SETTING (e.g. the default is 5 seconds), this drawback would appear in some very weirdly long-running flush that was being executed already. This is why the PR has this solution.
2. To avoid the drawback of the first solution, we could "chain" a second flush request to the first ongoing request. And make the first request, when finished, execute any chained requests. That way, we do not have a thread waiting on the flushLock. Drawback: complexity vs the other approaches.
3. We remove the wasActive smartness. Drawback: every SHARD_MEMORY_INTERVAL_TIME_SETTING ms, there will be a possible no-op flush request scheduled.

Feel free to tell us your opinion on the selected approach and any other thoughts you may have.

[henningandersen]

It does look like the old synced-flush would use waitIfOngoing=true, see here (and this PR). However, I wonder if we could let flush return a boolean if it attempted the flush or not - and if not, set active back to true?

A more straightforward solution could also be to move the setting of active to be after indexing is done rather than before? This also affects scheduledRefresh, but I think it is fine to not consider an indexing request that is ongoing active there too. I think I'd prefer that option, unless I am overseeing something.

[fcofdez]

A more straightforward solution could also be to move the setting of active to be after indexing is done rather than before? This also affects scheduledRefresh, but I think it is fine to not consider an indexing request that is ongoing active there too. I think I'd prefer that option, unless I am overseeing something.

I'm not sure if this solves the issue? Theoretically we could still miss the last flush if the first flush is still running after the remaining documents have been indexed. But maybe I'm missing something here.

[henningandersen]

I'm not sure if this solves the issue? Theoretically we could still miss the last flush if the first flush is still running after the remaining documents have been indexed. But maybe I'm missing something here.

You might be right 🙂 . The idea would be that by marking active after the indexing has occurred, the next round of flushOnIdle would know that it needs to flush?

[kingherc]

Hi! Thanks for the awesome conversation. I think I agree with @fcofdez . If we follow that approach @henningandersen , I so see a rare situation where:

• There's an ongoing flush of docs [1,N]
• A new doc N+1 is indexed and sets active = true
• The next flushOnIdle() is called, which sets active = false, and tries another flush request, which fails because of the ongoing flush.
• The ongoing flush finishes, but has missed N+1 document.
• Any next scheduled flushOnIdle() will not try flush requests since active = false.
  Right?

However, I wonder if we could let flush return a boolean if it attempted the flush or not - and if not, set active back to true?

I think this is also a nice solution. I can try that if you agree.

[fcofdez]

Just to add more context here, this only affects in cases where we stop indexing after the latest flush is skipped (a rare edge case)

I think this is also a nice solution. I can try that if you agree.

👍 let's try that.

[henningandersen]

Makes sense, the active flag would need to be cleared differently too (inside flush) if were to pursue the solution where we set active after indexing.

[kingherc]

Hi @fcofdez , @henningandersen . Thanks for the conversation. The method where if flush does not wait for the ongoing flush, it returns false, and we set the active flag back to true, works. I did that -- feel free to review the PR.

[fcofdez]

It looks like there are some related test failures, additionally we usually avoid force-pushing since Github gets confused and it hides/removes some review comments.

[kingherc]

It looks like there are some related test failures, additionally we usually avoid force-pushing since Github gets confused and it hides/removes some review comments.

Fixed the test.

Oh, about the force pushes, I will avoid from now on. Either way the commits get squashed when merging the PR, so indeed I do not see a reason now why I did it :)
Did you miss adding a comment? if yes, please mention it again please.

[fcofdez]

Oh, about the force pushes, I will avoid from now on. Either way the commits get squashed when merging the PR, so indeed I do not see a reason now why I did it :)

No worries, it's just a trade-off, sometimes it's easier to review a set of clean commits, but I'm not sure if GitHub will ever fix the force-push issue 🤔

[henningandersen]

This directions looks good to me. I have a few comments that I'd like to see addressed though.

[fcofdez]

I left a small comment, the direction looks good.

[henningandersen]

I think there is a problem with the new test, otherwise this looks good.

[henningandersen]

LGTM.

[fcofdez]

LGTM 👍

[kingherc]

@elasticmachine run elasticsearch-ci/part-1 please