Skip TRANSLOG stage for searchable snapshots recovery stage #67697
Conversation
This commit introduces a change where searchable snapshots skip the TRANSLOG stage. Since elastic#65531 was introduced, the cleanFiles peer recovery stage is blocked until the prewarming completes (this is done to avoid search latency spikes due to a cold cache). In that phase, the RecoveryState stage is TRANSLOG which can be confusing as we don't replay any ops during searchable snapshots recoveries. In order to avoid that confusion we transition directly to FINALIZE stage.
…-peer-recovery-state-new
fcofdez
added
Team:Distributed (Obsolete)
|
Pinging @elastic/es-distributed (Team:Distributed) |
| // filter for relocations that are not in stage FINALIZE (they could end up in this stage without progress for good if the | ||
| // target node does not have enough cache space available to hold the primary completely | ||
| .filter(recoveryState -> recoveryState.getSourceNode() != null && recoveryState.getStage() != RecoveryState.Stage.FINALIZE) | ||
| .filter(recoveryState -> recoveryState.getSourceNode() != null) |
There was a problem hiding this comment.
Maybe we could check at the end that the pre-warming phase ended?
There was a problem hiding this comment.
Yea we'd have to do something in that direction, otherwise https://github.com/elastic/elasticsearch/pull/67697/files#diff-be91049c27fa011e281944d790285ad97bbd72466aeeed55525c5b99061c9e13L113 will trip for small cache sizes that are randomly chosen in the parent test suite because we will get stuck in FINALIZE forever right?
There was a problem hiding this comment.
I think this is a neat change in general. I also couldn't find anything that is broken by it.
I'd like someone else to have a look here as well though. I couldn't find any side effects of hacking the stage like this outside of the assertion issues you fixed though and the behavior makes logical sense to me.
Maybe @henningandersen has a sec to double check that we can go this route in general?
| @@ -101,6 +101,11 @@ public static Stage fromId(byte id) { | |||
| } | |||
| } | |||
|
|
|||
| public enum IndexType { | |||
| REGULAR, | |||
| SEARCHABLE_SNAPSHOT; | |||
There was a problem hiding this comment.
This is a little nasty, I'm not 100% sure what our policy is here but I think we should avoid leaking searchable snapshots this explicitly in here. Maybe we could call this "NO_TRANSLOG" or so?
| // filter for relocations that are not in stage FINALIZE (they could end up in this stage without progress for good if the | ||
| // target node does not have enough cache space available to hold the primary completely | ||
| .filter(recoveryState -> recoveryState.getSourceNode() != null && recoveryState.getStage() != RecoveryState.Stage.FINALIZE) | ||
| .filter(recoveryState -> recoveryState.getSourceNode() != null) |
There was a problem hiding this comment.
Yea we'd have to do something in that direction, otherwise https://github.com/elastic/elasticsearch/pull/67697/files#diff-be91049c27fa011e281944d790285ad97bbd72466aeeed55525c5b99061c9e13L113 will trip for small cache sizes that are randomly chosen in the parent test suite because we will get stuck in FINALIZE forever right?
|
BTW: I also wonder if we really need to be this invasive here. Maybe we could simply add a filter to for the stage to render in APIs (e.g. WDYT? |
|
I wonder if we could instead move setting stage to |
++ that seems pretty side effect free and quick. |
But in that case the recovery stage would remain in
I like this approach, I'll go ahead with that. |
|
I think we could stay in |
|
@elasticmachine update branch |
|
Sorry, this fell through the cracks. I've opened a new PR with the suggested approach (#68680) |
This commit introduces a change where searchable snapshots
skip the RecoveryState TRANSLOG stage. Since #65531 was introduced, the
cleanFiles peer recovery stage is blocked until the prewarming
completes (this is done to avoid search latency spikes due to a cold
cache). In that phase, the RecoveryState stage is
TRANSLOG which can be confusing as we don't replay
any ops during searchable snapshots recoveries. In order
to avoid that confusion we transition directly to
FINALIZE stage.