-
Notifications
You must be signed in to change notification settings - Fork 24.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add peer recovery planners that take into account available snapshots #75840
Add peer recovery planners that take into account available snapshots #75840
Conversation
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
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/indices/recovery/plan/ShardRecoveryPlanner.java
Outdated
Show resolved
Hide resolved
@elasticmachine run elasticsearch-ci/part-2 (it was a docker-compose problem) |
Pinging @elastic/es-distributed (Team:Distributed) |
@DaveCTurner I added the wiring + feature flag, could you take a look when you have the chance? Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/indices/recovery/plan/ShardRecoveryPlanners.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/indices/recovery/RecoverySettings.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/indices/recovery/RecoverySettings.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/indices/recovery/RecoverySettings.java
Outdated
Show resolved
Hide resolved
server/src/test/java/org/elasticsearch/indices/recovery/RecoverySourceHandlerTests.java
Outdated
Show resolved
Hide resolved
…snapshot-planners
…snapshot-planners
Co-authored-by: David Turner <david.turner@elastic.co>
Co-authored-by: David Turner <david.turner@elastic.co>
…snapshot-planners
…snapshot-planners
This reverts commit fde53c9.
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
…snapshot-planners
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting close, I left only tiny suggestions.
server/src/main/java/org/elasticsearch/indices/recovery/RecoverySettings.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/indices/recovery/RecoverySettings.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/indices/recovery/plan/ShardSnapshotsService.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/indices/recovery/plan/ShardSnapshotsService.java
Outdated
Show resolved
Hide resolved
} | ||
|
||
ShardSnapshot latestSnapshot = availableSnapshots.stream() | ||
.max(Comparator.comparingLong(ShardSnapshot::getStartedAt)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Show resolved
Hide resolved
final long totalSize = shardRecoveryPlan.getTotalSize(); | ||
final long existingTotalSize = shardRecoveryPlan.getExistingSize(); | ||
|
||
if (logger.isTraceEnabled()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 nice work keeping this logging intact.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry just a handful more small things that struck me from looking at #76114.
server/src/main/java/org/elasticsearch/indices/recovery/RecoverySettings.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/indices/recovery/plan/ShardSnapshotsService.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/indices/recovery/plan/ShardSnapshotsService.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
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
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.