Sending operations concurrently in peer recovery by dnhatn · Pull Request #58018 · elastic/elasticsearch

dnhatn · 2020-06-12T03:36:07Z

Today, we send operations in phase2 of peer recoveries batch by batch sequentially. Normally that's okay as we should have a fairly small of operations in phase 2 due to the file-based threshold. However, if phase1 takes a lot of time and we are actively indexing, then phase2 can have a lot of operations to replay.

With this change, we will send multiple batches concurrently (defaults to 1) to reduce the recovery time.

Relates #58011

elasticmachine · 2020-06-12T03:36:10Z

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

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

DaveCTurner

I think this is a little risky; I've seen cases where it appeared that the load from recovery phase 2 already has a performance impact on indexing into other active shards on the target node. It's certainly nice to have the option of more concurrency in phase 2 but I think we should be cautious about doing so by default.

I'll link such a case to this PR.

original-brownbear

One question on the mechanics of this, maybe I'm missing something here.

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

original-brownbear

Looks good to me technically :)

I share Davids concern though. Even more so if we start moving the handler for this to WRITE which under load could cause additional trouble from more rejections in a case like the one David linked. Maybe we should just default to the current behavior for now and make this optional?

dnhatn · 2020-07-06T15:57:32Z

@DaveCTurner @original-brownbear Thanks for reviewing. I've added a new recovery setting that controls the number of operation chunks sent in parallel. Could you please take another look?

original-brownbear · 2020-07-06T17:40:23Z

@dnhatn will take a look soon, but FYI there's some random CS failure here in :test:framework:checkstyleMain :)

original-brownbear

LGTM :)

docs/reference/modules/indices/recovery.asciidoc

DaveCTurner

Docs & naming LGTM; I haven't reviewed the code change in detail but I think Armin did.

dnhatn · 2020-07-07T17:40:39Z

run elasticsearch-ci/2

dnhatn · 2020-07-07T19:11:44Z

@elasticmachine update branch

dnhatn · 2020-07-07T21:58:44Z

@original-brownbear @DaveCTurner Thanks for reviewing.

Today, we send operations in phase2 of peer recoveries batch by batch sequentially. Normally that's okay as we should have a fairly small of operations in phase 2 due to the file-based threshold. However, if phase1 takes a lot of time and we are actively indexing, then phase2 can have a lot of operations to replay. With this change, we will send multiple batches concurrently (defaults to 1) to reduce the recovery time. Backport of #58018

We need to use a concurrent collection to keep track of the shipped operations as they can arrive concurrently since #58018. Relates #58018

The recovery chunk size setting was injected in #58018, but too aggressively and broke several tests. This change removes that random injection. Relates #58018

Sending operations concurrently in peer recovery

a2103a7

dnhatn added >enhancement :Distributed/Recovery Anything around constructing a new shard, either from a local or a remote source. v8.0.0 v7.9.0 labels Jun 12, 2020

dnhatn requested review from original-brownbear and ywelsch June 12, 2020 03:36

elasticmachine added the Team:Distributed Meta label for distributed team. label Jun 12, 2020

dnhatn commented Jun 12, 2020

View reviewed changes

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

fix test

7707c33

DaveCTurner reviewed Jun 12, 2020

View reviewed changes

original-brownbear reviewed Jun 12, 2020

View reviewed changes

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

original-brownbear reviewed Jun 12, 2020

View reviewed changes

original-brownbear mentioned this pull request Jun 12, 2020

Fix Running TranslogOps on CS Thread #58056

Merged

dnhatn mentioned this pull request Jun 30, 2020

Improve peer recovery performance in active indexing #58011

Closed

ywelsch removed their request for review July 3, 2020 06:45

dnhatn added 4 commits July 6, 2020 10:08

Merge branch 'master' into send-ops-concurrently

e1332b3

New setting

a28c2e3

Use generic to write operations

d34db4b

fix setting

a564188

dnhatn requested review from DaveCTurner and original-brownbear July 6, 2020 15:57

reduce random setting upper bound

44f83d6

dnhatn added 2 commits July 6, 2020 14:15

Merge branch 'master' into send-ops-concurrently

1a6b630

stylecheck

f0c2d71

original-brownbear approved these changes Jul 6, 2020

View reviewed changes

docs/reference/modules/indices/recovery.asciidoc Outdated Show resolved Hide resolved

DaveCTurner reviewed Jul 7, 2020

View reviewed changes

docs/reference/modules/indices/recovery.asciidoc Outdated Show resolved Hide resolved

docs/reference/modules/indices/recovery.asciidoc Outdated Show resolved Hide resolved

docs/reference/modules/indices/recovery.asciidoc Outdated Show resolved Hide resolved

dnhatn added 3 commits July 7, 2020 10:36

rewording

e4f1fbf

rewording

a7f1df3

Merge branch 'master' into send-ops-concurrently

50c13f7

dnhatn requested a review from DaveCTurner July 7, 2020 14:47

DaveCTurner approved these changes Jul 7, 2020

View reviewed changes

fix test after removing uid from translog operation

ef09c18

Merge branch 'master' into send-ops-concurrently

ff7e57b

dnhatn merged commit 961db31 into elastic:master Jul 7, 2020

dnhatn deleted the send-ops-concurrently branch July 7, 2020 22:00

dnhatn mentioned this pull request Jul 7, 2020

Sending operations concurrently in peer recovery #59198

Merged

dnhatn added a commit that referenced this pull request Jul 8, 2020

Fix testSendSnapshotSendsOps

865b6b5

We need to use a concurrent collection to keep track of the shipped operations as they can arrive concurrently since #58018. Relates #58018

dnhatn added a commit that referenced this pull request Jul 8, 2020

Fix testSendSnapshotSendsOps

00c859b

We need to use a concurrent collection to keep track of the shipped operations as they can arrive concurrently since #58018. Relates #58018

dnhatn added a commit that referenced this pull request Jul 8, 2020

Remove random of recovery chunk size setting

e50a033

The recovery chunk size setting was injected in #58018, but too aggressively and broke several tests. This change removes that random injection. Relates #58018

dnhatn added a commit that referenced this pull request Jul 8, 2020

Remove random of recovery chunk size setting

54e0c95

The recovery chunk size setting was injected in #58018, but too aggressively and broke several tests. This change removes that random injection. Relates #58018

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

Conversation

dnhatn commented Jun 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Jun 12, 2020

Uh oh!

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

original-brownbear left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

original-brownbear left a comment

Choose a reason for hiding this comment

Uh oh!

dnhatn commented Jul 6, 2020

Uh oh!

original-brownbear commented Jul 6, 2020

Uh oh!

original-brownbear left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

dnhatn commented Jul 7, 2020

Uh oh!

dnhatn commented Jul 7, 2020

Uh oh!

dnhatn commented Jul 7, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dnhatn commented Jun 12, 2020 •

edited

Loading