Add peer recovery planners that take into account available snapshots by fcofdez · Pull Request #75840 · elastic/elasticsearch

fcofdez · 2021-07-29T13:30:18Z

This commit adds a new set of classes that would compute a peer
recovery plan, based on source files + target files + available
snapshots. When possible it would try to maximize the number of
files used from a snapshot.

This commit adds a new set of classes that would compute a peer recovery plan, based on source files + target files + available snapshots. When possible it would try to maximize the number of files used from a snapshot. This commit doesn't wire these classes yet to avoid adding additional noise.

server/src/main/java/org/elasticsearch/indices/recovery/plan/PlannerService.java

server/src/main/java/org/elasticsearch/indices/recovery/plan/ShardRecoveryPlanner.java

fcofdez · 2021-07-29T14:42:58Z

@elasticmachine run elasticsearch-ci/part-2

(it was a docker-compose problem)

elasticmachine · 2021-07-29T16:21:43Z

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

…snapshot-planners

fcofdez · 2021-07-30T14:19:32Z

@DaveCTurner I added the wiring + feature flag, could you take a look when you have the chance? Thanks!

DaveCTurner

Thanks @fcofdez for wiring everything up. I think I understand how it all fits together now, but I left a few suggestions for simplifying things in areas where we aren't using all the generality we're introducing. I haven't gone right down to the lowest levels of detail yet because I think things will still move around quite a bit.

server/src/main/java/org/elasticsearch/indices/recovery/plan/RecoveryPlannerService.java

server/src/main/java/org/elasticsearch/indices/recovery/plan/ShardRecoveryPlanners.java

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

server/src/test/java/org/elasticsearch/indices/recovery/RecoverySourceHandlerTests.java

docs/reference/modules/indices/recovery.asciidoc

…snapshot-planners

Co-authored-by: David Turner <david.turner@elastic.co>

…snapshot-planners

This reverts commit fde53c9.

fcofdez · 2021-08-04T15:53:52Z

We discussed offline the pros and cons of using a single repository specified as a node setting and we reached the conclusion that this might be limiting in terms of flexibility. We agreed on introducing a new repository setting that would enable using the latest available snapshot from that repository explicitly. We would implement this in a subsequent PR. cc @DaveCTurner @henningandersen

…snapshot-planners

DaveCTurner

Getting close, I left only tiny suggestions.

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

server/src/main/java/org/elasticsearch/indices/recovery/plan/ShardSnapshotsService.java

DaveCTurner · 2021-08-09T08:53:08Z

...r/src/main/java/org/elasticsearch/indices/recovery/plan/SnapshotsRecoveryPlannerService.java

+        }
+
+        ShardSnapshot latestSnapshot = availableSnapshots.stream()
+            .max(Comparator.comparingLong(ShardSnapshot::getStartedAt))


In a follow-up I'd like us to push this .max calculation through TransportGetShardSnapshotAction and into IndexSnapshotsService: as it stands we might retrieve a SnapshotInfo from each repository but really if we checked them in time order we could do them in reverse time order and stop as soon as we've found the one we'll be using here.

If we can't do that before 7.15 then it's probably ok, it'll just leave a bit of BwC cruft.

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

DaveCTurner · 2021-08-09T09:22:17Z

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

+        final long totalSize = shardRecoveryPlan.getTotalSize();
+        final long existingTotalSize = shardRecoveryPlan.getExistingSize();
+
+        if (logger.isTraceEnabled()) {


👍 nice work keeping this logging intact.

DaveCTurner

Sorry just a handful more small things that struck me from looking at #76114.

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

server/src/main/java/org/elasticsearch/indices/recovery/plan/ShardSnapshotsService.java

DaveCTurner

LGTM 👍

fcofdez · 2021-08-09T12:04:34Z

Thanks David!

This commit adds a new set of classes that would compute a peer recovery plan, based on source files + target files + available snapshots. When possible it would try to maximize the number of files used from a snapshot. It uses repositories with `use_for_peer_recovery` setting set to true. It adds a new recovery setting `indices.recovery.use_snapshots` Relates elastic#73496 Backport of elastic#75840

…#76239) This commit adds a new set of classes that would compute a peer recovery plan, based on source files + target files + available snapshots. When possible it would try to maximize the number of files used from a snapshot. It uses repositories with `use_for_peer_recovery` setting set to true. It adds a new recovery setting `indices.recovery.use_snapshots` Relates #73496 Backport of #75840

elasticsearchmachine added the v8.0.0 label Jul 29, 2021

Renaming mistake

63269b1

fcofdez commented Jul 29, 2021

View reviewed changes

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

fcofdez commented Jul 29, 2021

View reviewed changes

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

fcofdez mentioned this pull request Jul 29, 2021

Replicate or relocate data via snapshot #73496

Closed

8 tasks

fcofdez requested a review from DaveCTurner July 29, 2021 16:21

fcofdez added v7.15.0 :Distributed/Recovery Anything around constructing a new shard, either from a local or a remote source. >enhancement Team:Distributed Meta label for distributed team. labels Jul 29, 2021

fcofdez marked this pull request as ready for review July 29, 2021 16:21

fcofdez added 4 commits July 30, 2021 14:18

Wire everything together

7302d3e

Merge remote-tracking branch 'origin/master' into peer-recovery-from-…

7e12ecf

…snapshot-planners

Renaming mistake

0f76146

Make setting static

2fecdc9

fcofdez added 2 commits July 30, 2021 21:34

Remove dead code

9bf26ca

Add a setting to specify which repository to use during peer recoveries

a4f803e

DaveCTurner reviewed Aug 3, 2021

View reviewed changes

fcofdez added 8 commits August 3, 2021 16:06

Merge remote-tracking branch 'origin/master' into peer-recovery-from-…

5029e16

…snapshot-planners

Simplify code

b1d5520

Merge remote-tracking branch 'origin/master' into peer-recovery-from-…

eb86720

…snapshot-planners

Fix error

a44509e

Add dynamic settings

ce1c94b

Add test to RecoverySourceHandlerTests

661b51e

Generalize expected error

4b43100

Precommit

4f6c493

fcofdez and others added 8 commits August 4, 2021 12:18

Update docs/reference/modules/indices/recovery.asciidoc

4e65240

Co-authored-by: David Turner <david.turner@elastic.co>

Update docs/reference/modules/indices/recovery.asciidoc

21772c7

Co-authored-by: David Turner <david.turner@elastic.co>

Remove unnecessary annotation

9e9174f

Merge remote-tracking branch 'origin/master' into peer-recovery-from-…

2c58137

…snapshot-planners

Support only 1 repository

fde53c9

Merge remote-tracking branch 'origin/master' into peer-recovery-from-…

ceeae01

…snapshot-planners

Revert "Support only 1 repository"

05bdcd9

This reverts commit fde53c9.

Take into account repo path

34047af

fcofdez requested a review from DaveCTurner August 4, 2021 15:53

fcofdez added 5 commits August 4, 2021 22:09

Take into account node versions

ddf8cfd

Merge remote-tracking branch 'origin/master' into peer-recovery-from-…

b09b0b5

…snapshot-planners

Merge remote-tracking branch 'origin/master' into peer-recovery-from-…

9a77bf3

…snapshot-planners

Simplify

ff1f120

Get rid of repository setting and use per repository setting

18959b6

DaveCTurner reviewed Aug 9, 2021

View reviewed changes

Review comments

dc83c55

fcofdez requested a review from DaveCTurner August 9, 2021 10:57

DaveCTurner approved these changes Aug 9, 2021

View reviewed changes

fcofdez merged commit 3c8b9a6 into elastic:master Aug 9, 2021

fcofdez added the backport pending label Aug 9, 2021

fcofdez mentioned this pull request Aug 9, 2021

[7.x] Add peer recovery planners that take into account available snapshots #76239

Merged

fcofdez removed the backport pending label Aug 9, 2021

fcofdez mentioned this pull request Aug 10, 2021

Compute latest snapshot directly in TransportGetShardSnapshotAction #76254

Merged

jakelandis added v8.0.0-alpha2 and removed v8.0.0 labels Sep 15, 2021

Conversation

fcofdez commented Jul 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fcofdez commented Jul 29, 2021

Uh oh!

elasticmachine commented Jul 29, 2021

Uh oh!

fcofdez commented Jul 30, 2021

Uh oh!

DaveCTurner 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!

fcofdez commented Aug 4, 2021

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DaveCTurner Aug 9, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DaveCTurner Aug 9, 2021

Choose a reason for hiding this comment

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

fcofdez commented Aug 9, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

fcofdez commented Jul 29, 2021 •

edited

Loading