Noop peer recoveries on closed index #41400

dnhatn · 2019-04-21T22:35:10Z

If users close an index to change some non-dynamic index settings, then the current implementation forces replicas of that closed index to copy over segment files from the primary. With this change, we make peer recoveries of closed index skip both phases.

Relates #33888

Co-authored-by: Yannick Welsch yannick@welsch.lu

elasticmachine · 2019-04-21T22:35:11Z

Pinging @elastic/es-distributed

readonly engine does not support synced-flush

server/src/main/java/org/elasticsearch/cluster/metadata/MetaDataIndexStateService.java

dnhatn · 2019-04-27T02:52:53Z

@ywelsch I push changes. Can you take another look? Thank you!

ywelsch · 2019-04-29T12:09:08Z

The more I came to think of this, I wonder if it is easier, as a first step, to avoid the issue of closed replicated indices doing file-based recovery by just changing hasCompleteHistoryOperations, which is not properly implemented on a read-only engine
https://github.com/elastic/elasticsearch/compare/master...ywelsch:noop-recoveries-on-closed-index?expand=1
This avoids the need for making the closing logic more complicated, and also avoids the need to introduce more code that makes us rely on sync flush markers.

henningandersen

Thanks @dnhatn , I left a few comments to consider.

server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java

server/src/main/java/org/elasticsearch/cluster/metadata/MetaDataIndexStateService.java

server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java

...org/elasticsearch/action/admin/indices/close/TransportVerifyShardBeforeCloseActionTests.java

dnhatn · 2019-04-29T15:10:25Z

This avoids the need for making the closing logic more complicated, and also avoids the need to introduce more code that makes us rely on sync flush markers.

@ywelsch Great idea! Sadly, this change does not play well with closed follower indices.

Having primary and replica of a follower index available
Index seq-0 then flush, the local checkpoint is 1 on both primary and replica
Shutdown the node with replica
Index seq-2 (to the primary only) then close an index. The local checkpoint on the primary is still 0.
Start the node with the replica. With this change, it will perform a noop peer recovery then it won't have seq-2.

dnhatn · 2019-04-29T18:29:44Z

@henningandersen Discussed with Yannick on another channel, we agreed to go with Yannick's proposal; however, we need to strengthen the operation-based condition in ReadOnlyEngine (addressed in 35c527b).

@ywelsch @henningandersen Can you please take another look? Thank you!

henningandersen

LGTM.

Thanks @dnhatn

henningandersen · 2019-04-29T18:53:02Z

server/src/test/java/org/elasticsearch/indices/state/CloseIndexIT.java

@@ -338,6 +340,37 @@ public void testCloseIndexWaitForActiveShards() throws Exception {
        assertIndexIsClosed(indexName);
    }

+    public void testNoopPeerRecoveriesWhenIndexClosed() throws Exception {


Would be nice to also test the scenario you described here:

#41400 (comment)

where we expect file based recovery and verify same docs on all shards.

@henningandersen Good suggestion. However we can't test that scenario for now since closing a follower index with gaps in sequence number will make all its shard unassigned; hence no peer recovery will be performed.

@dnhatn Can you implement the test scenario that you've described for regular indices (instead of follower index)? It will then show that a closed replica index that is missing some docs IS doing a file-based recovery.

I added a test in b50d3f2.

dnhatn · 2019-04-30T21:17:53Z

@elasticmachine test this please

ywelsch

LGTM

ywelsch · 2019-05-02T14:20:33Z

server/src/test/java/org/elasticsearch/indices/state/CloseIndexIT.java

+        }
+    }
+
+    public void testRecoverExistingReplica() throws Exception {


perhaps add a comment that says that this tests recovery of a replica of a closed index that has some docs missing that were on the primary, leading to a file-based recovery

When an index is closed, we expect primary and replicas to be identical. This commit improves the gateway replica shard allocator to consider shards with identical sequence numbers sync'ed for closed indices. This ensures that we will pick a fast recovery regardless of whether synced flush was performed prior to closing an index. Relates elastic#41400 and elastic#33888

Added integration test validating that fast recovery is made for closed indices when multiple shard copies can be chosen from. Fixed InternalTestCluster to allow doing operations inside onStopped() when using restartXXXNode(). Relates elastic#41400 and elastic#33888

dnhatn · 2019-05-03T15:38:40Z

@ywelsch @henningandersen Thanks for reviewing.

If users close an index to change some non-dynamic index settings, then the current implementation forces replicas of that closed index to copy over segment files from the primary. With this change, we make peer recoveries of closed index skip both phases. Relates #33888 Co-authored-by: Yannick Welsch <yannick@welsch.lu>

This is a first step away from sync-ids. We now check if replica and primary are identical using sequence numbers when determining where to allocate a replica shard. If an index is no longer indexed into, issuing a regular flush will now be enough to ensure a no-op recovery is done. This has the nice side-effect of ensuring that closed indices and frozen indices choose existing shard copies with identical data over file-overlap comparison, increasing the chance that we end up doing a no-op recovery (only no-op and file-based recovery is supported by closed indices). Relates elastic#41400 and elastic#33888 Supersedes elastic#41784

If users close an index to change some non-dynamic index settings, then the current implementation forces replicas of that closed index to copy over segment files from the primary. With this change, we make peer recoveries of closed index skip both phases. Relates elastic#33888 Co-authored-by: Yannick Welsch <yannick@welsch.lu>

Synced flush indices before closing

783cd1e

dnhatn added >enhancement :Distributed/Distributed A catch all label for anything in the Distributed Area. If you aren't sure, use this one. v8.0.0 v7.2.0 labels Apr 21, 2019

dnhatn requested a review from ywelsch April 21, 2019 22:35

tlrx mentioned this pull request Apr 21, 2019

Replicate closed indices #33888

Closed

50 tasks

dnhatn added 2 commits April 21, 2019 20:05

move prepare action to internal engine

1c485e0

readonly engine does not support synced-flush

synced-flush post_recovery state

da0f7fe

dnhatn requested a review from henningandersen April 23, 2019 14:19

ywelsch reviewed Apr 26, 2019

View reviewed changes

server/src/main/java/org/elasticsearch/cluster/metadata/MetaDataIndexStateService.java Outdated Show resolved Hide resolved

dnhatn added 2 commits April 26, 2019 17:08

Merge branch 'master' into synced-flush-closed-index

e2bdbb8

reuse syncId

9ed30cd

dnhatn requested a review from ywelsch April 27, 2019 02:52

dnhatn added 7 commits April 26, 2019 23:00

wording

68c7d0c

wording

fe9a294

fix tests

1946d92

Merge branch 'master' into synced-flush-closed-index

95d3536

simplify syncId logic

a0f8d03

fix comment

732049e

Merge branch 'master' into synced-flush-closed-index

f3228d3

henningandersen reviewed Apr 29, 2019

View reviewed changes

dnhatn and others added 4 commits April 29, 2019 13:37

Merge branch 'master' into synced-flush-closed-index

7bdece5

backout sync id

38bd362

Noop peer recoveries on closed index

042ceb4

strengthen operation-based condition

35c527b

dnhatn changed the title ~~Synced flush indices before closing~~ Noop peer recoveries on closed index Apr 29, 2019

dnhatn requested a review from henningandersen April 29, 2019 18:29

henningandersen approved these changes Apr 29, 2019

View reviewed changes

dnhatn added 2 commits April 30, 2019 13:02

Merge branch 'master' into synced-flush-closed-index

685c5cd

add recover existing replica test

b50d3f2

ywelsch approved these changes May 2, 2019

View reviewed changes

henningandersen mentioned this pull request May 3, 2019

Closed index replica allocation #41784

Closed

dnhatn added 2 commits May 3, 2019 08:46

add test comment

c1d3324

Merge branch 'master' into synced-flush-closed-index

aa2d525

dnhatn merged commit c7df2b8 into elastic:master May 3, 2019

dnhatn deleted the synced-flush-closed-index branch May 3, 2019 15:39

dnhatn added the backport pending label May 3, 2019

dnhatn removed the backport pending label May 3, 2019

henningandersen mentioned this pull request May 24, 2019

Replica allocation consider no-op #42518

Closed

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Noop peer recoveries on closed index #41400

Noop peer recoveries on closed index #41400

dnhatn commented Apr 21, 2019 •

edited

Loading

elasticmachine commented Apr 21, 2019

dnhatn commented Apr 27, 2019

ywelsch commented Apr 29, 2019

henningandersen left a comment

dnhatn commented Apr 29, 2019

dnhatn commented Apr 29, 2019

henningandersen left a comment

henningandersen Apr 29, 2019

dnhatn Apr 29, 2019

ywelsch Apr 30, 2019

dnhatn Apr 30, 2019

dnhatn commented Apr 30, 2019

ywelsch left a comment

ywelsch May 2, 2019

dnhatn commented May 3, 2019

Noop peer recoveries on closed index #41400

Noop peer recoveries on closed index #41400

Conversation

dnhatn commented Apr 21, 2019 • edited Loading

elasticmachine commented Apr 21, 2019

dnhatn commented Apr 27, 2019

ywelsch commented Apr 29, 2019

henningandersen left a comment

Choose a reason for hiding this comment

dnhatn commented Apr 29, 2019

dnhatn commented Apr 29, 2019

henningandersen left a comment

Choose a reason for hiding this comment

henningandersen Apr 29, 2019

Choose a reason for hiding this comment

dnhatn Apr 29, 2019

Choose a reason for hiding this comment

ywelsch Apr 30, 2019

Choose a reason for hiding this comment

dnhatn Apr 30, 2019

Choose a reason for hiding this comment

dnhatn commented Apr 30, 2019

ywelsch left a comment

Choose a reason for hiding this comment

ywelsch May 2, 2019

Choose a reason for hiding this comment

dnhatn commented May 3, 2019

dnhatn commented Apr 21, 2019 •

edited

Loading