Use global checkpoint as starting seq in ops-based recovery by dnhatn · Pull Request #43463 · elastic/elasticsearch

dnhatn · 2019-06-21T03:42:24Z

Today we use the local checkpoint of the safe commit on replicas to determine whether we can perform a sequence number based recovery. While this is a good choice due to its simplicity, it replies on flushing which should not happen frequently.

This change increases the chance of sequence number based recoveries by using the global checkpoint on the target as the starting sequence number when possible.

elasticmachine · 2019-06-21T03:42:26Z

Pinging @elastic/es-distributed

dnhatn · 2019-06-21T03:42:44Z

This PR is still WIP but I opened this to get your feedback on the approach.
@ywelsch @henningandersen @DaveCTurner Could you please have a look when you have some cycles? Thank you!

henningandersen

Thanks @dnhatn , I left a few initial comments.

server/src/main/java/org/elasticsearch/index/shard/IndexShard.java

server/src/main/java/org/elasticsearch/indices/recovery/RecoveryTarget.java

server/src/main/java/org/elasticsearch/indices/recovery/RecoverySourceHandler.java

ywelsch

Thanks for picking this up. I've left some preliminary comments.

server/src/main/java/org/elasticsearch/index/shard/IndexShard.java

server/src/main/java/org/elasticsearch/indices/recovery/RecoveryTarget.java

server/src/main/java/org/elasticsearch/indices/recovery/RecoverySourceHandler.java

server/src/main/java/org/elasticsearch/indices/recovery/RecoveryTarget.java

server/src/main/java/org/elasticsearch/indices/recovery/StartRecoveryRequest.java

server/src/main/java/org/elasticsearch/indices/recovery/RecoveryTarget.java

This reverts commit f8c0b15.

…checkpoint

dnhatn · 2019-06-27T03:18:00Z

Talked to Yannick on another channel, we preferred to make this change altogether with the peer recovery retention leases work. Therefore, this PR will go to the feature branch (peer-recovery-retention-leases).

@henningandersen @ywelsch @DaveCTurner This is ready for another round. Can you please take another look? Thank you!

ywelsch · 2019-06-27T15:51:32Z

@dnhatn there is a relevant test failure here I think

…checkpoint

ywelsch

LGTM

server/src/main/java/org/elasticsearch/index/shard/IndexShard.java

This reverts commit 6ec4e80.

dnhatn · 2019-07-23T13:50:06Z

run elasticsearch-ci/packaging-sample

…checkpoint

dnhatn · 2019-07-23T16:28:40Z

@ywelsch @henningandersen @DaveCTurner Thank you for reviewing. Yannick, sorry for many iterations in this PR. I should have done better here.

Today we use the local checkpoint of the safe commit on replicas as the starting sequence number of operation-based peer recovery. While this is a good choice due to its simplicity, we need to share this information between copies if we use retention leases in peer recovery. We can avoid this extra work if we use the global checkpoint as the starting sequence number. With this change, we will try to recover replica locally up to the global checkpoint before performing peer recovery. This commit should also increase the chance of operation-based recovery.

Relates #43463

… step (#44781) If we force allocate an empty or stale primary, the global checkpoint on replicas might be higher than the primary's as the local recovery step (introduced in #43463) loads the previous (stale) global checkpoint into ReplicationTracker. There's no issue with the retention leases for a new lease with a higher term will supersede the stale one. Relates #43463

For closed and frozen indices, we should not recover shard locally up to the global checkpoint before performing peer recovery for that copy might be offline when the index was closed/frozen. Relates #43463 Closes #44855

Previously, if the metadata snapshot is empty (either no commit found or error), we won't compute the starting sequence number and use -2 to opt out the operation-based recovery. With #43463, we have a starting sequence number before reading the last commit. Thus, we need to reset it if we fail to snapshot the store. Closes #45072

Use global checkpoint as base for seq based recovery

f8c0b15

dnhatn added >enhancement WIP :Distributed/Recovery Anything around constructing a new shard, either from a local or a remote source. v8.0.0 v7.3.0 labels Jun 21, 2019

dnhatn requested review from DaveCTurner, henningandersen and ywelsch June 21, 2019 03:42

henningandersen reviewed Jun 24, 2019

View reviewed changes

ywelsch suggested changes Jun 24, 2019

View reviewed changes

DaveCTurner reviewed Jun 24, 2019

View reviewed changes

server/src/main/java/org/elasticsearch/indices/recovery/RecoveryTarget.java Outdated Show resolved Hide resolved

dnhatn added 3 commits June 25, 2019 14:09

Revert "Use global checkpoint as base for seq based recovery"

72458c9

This reverts commit f8c0b15.

Merge branch 'master' into recover-to-global-checkpoint

d6fe637

Recover locally first

ef454e2

dnhatn changed the base branch from master to peer-recovery-retention-leases June 26, 2019 18:00

dnhatn added 2 commits June 26, 2019 23:08

Merge branch 'peer-recovery-retention-leases' into recover-to-global-…

800b329

…checkpoint

add comment

c05d70d

dnhatn removed WIP v7.3.0 v8.0.0 labels Jun 27, 2019

dnhatn requested review from DaveCTurner, henningandersen and ywelsch June 27, 2019 03:18

dnhatn added 2 commits June 27, 2019 16:08

fix tests

c535d5f

Merge branch 'peer-recovery-retention-leases' into recover-to-global-…

ed024e7

…checkpoint

dnhatn added 5 commits July 22, 2019 08:31

combine with performRecoveryRestart

3280e64

more feedback

30ef0c6

log the global checkpoint

a40b5dd

adjust the total local to reflect the exact count

ed5d302

Merge branch 'peer-recovery-retention-leases' into recover-to-global-…

731af0e

…checkpoint

dnhatn requested a review from ywelsch July 22, 2019 14:39

fix translog recovery stats

e6923e8

ywelsch approved these changes Jul 23, 2019

View reviewed changes

server/src/main/java/org/elasticsearch/index/shard/IndexShard.java Show resolved Hide resolved

server/src/main/java/org/elasticsearch/index/shard/IndexShard.java Show resolved Hide resolved

dnhatn added 2 commits July 23, 2019 08:20

do not adjust

6ec4e80

Revert "do not adjust"

7a26f32

This reverts commit 6ec4e80.

Merge branch 'peer-recovery-retention-leases' into recover-to-global-…

1deddbe

…checkpoint

dnhatn merged commit d15684d into elastic:peer-recovery-retention-leases Jul 23, 2019

dnhatn deleted the recover-to-global-checkpoint branch July 23, 2019 16:47

dnhatn added the backport pending label Jul 23, 2019

dnhatn added a commit that referenced this pull request Jul 23, 2019

Adjust BWC for recovery translog stats

06d9be6

Relates #43463

dnhatn removed the backport pending label Jul 23, 2019

dnhatn mentioned this pull request Jul 24, 2019

Do not load global checkpoint to ReplicationTracker in local recovery step #44781

Merged

This was referenced Jul 25, 2019

Retain history for peer recovery using leases #41536

Closed

Failure in CloseWhileRelocatingShardsIT #44855

Closed

dnhatn mentioned this pull request Jul 25, 2019

Skip local recovery for closed or frozen indices #44887

Merged

dnhatn mentioned this pull request Aug 1, 2019

Reset starting seqno if fail to read last commit #45106

Merged

Conversation

dnhatn commented Jun 21, 2019

Uh oh!

elasticmachine commented Jun 21, 2019

Uh oh!

dnhatn commented Jun 21, 2019

Uh oh!

henningandersen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ywelsch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dnhatn commented Jun 27, 2019

Uh oh!

ywelsch commented Jun 27, 2019

Uh oh!

ywelsch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dnhatn commented Jul 23, 2019

Uh oh!

dnhatn commented Jul 23, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants