-
Notifications
You must be signed in to change notification settings - Fork 25.2k
Replica starts peer recovery with safe commit #28181
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
Changes from 1 commit
2509d17
9d7db40
c9152ea
2ff5354
d66ba4b
1eea7b1
9b25413
efb5149
2642d7e
3ef8bf5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,66 @@ | ||
/* | ||
* Licensed to Elasticsearch under one or more contributor | ||
* license agreements. See the NOTICE file distributed with | ||
* this work for additional information regarding copyright | ||
* ownership. Elasticsearch licenses this file to you under | ||
* the Apache License, Version 2.0 (the "License"); you may | ||
* not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, | ||
* software distributed under the License is distributed on an | ||
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
* KIND, either express or implied. See the License for the | ||
* specific language governing permissions and limitations | ||
* under the License. | ||
*/ | ||
|
||
package org.elasticsearch.indices.recovery; | ||
|
||
import org.elasticsearch.common.io.stream.StreamInput; | ||
import org.elasticsearch.common.io.stream.StreamOutput; | ||
import org.elasticsearch.index.shard.ShardId; | ||
import org.elasticsearch.transport.TransportRequest; | ||
|
||
import java.io.IOException; | ||
|
||
final class RecoveryOpenSeqBasedEngineRequest extends TransportRequest { | ||
final long recoveryId; | ||
final ShardId shardId; | ||
final int totalTranslogOps; | ||
|
||
RecoveryOpenSeqBasedEngineRequest(long recoveryId, ShardId shardId, int totalTranslogOps) { | ||
this.recoveryId = recoveryId; | ||
this.shardId = shardId; | ||
this.totalTranslogOps = totalTranslogOps; | ||
} | ||
|
||
RecoveryOpenSeqBasedEngineRequest(StreamInput in) throws IOException { | ||
super(in); | ||
recoveryId = in.readLong(); | ||
shardId = ShardId.readShardId(in); | ||
totalTranslogOps = in.readVInt(); | ||
} | ||
|
||
@Override | ||
public void writeTo(StreamOutput out) throws IOException { | ||
super.writeTo(out); | ||
out.writeLong(recoveryId); | ||
shardId.writeTo(out); | ||
out.writeVInt(totalTranslogOps); | ||
} | ||
|
||
public long recoveryId() { | ||
return this.recoveryId; | ||
} | ||
|
||
public ShardId shardId() { | ||
return shardId; | ||
} | ||
|
||
public int totalTranslogOps() { | ||
return totalTranslogOps; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -188,7 +188,7 @@ public RecoveryResponse recoverToTarget() throws IOException { | |
runUnderPrimaryPermit(() -> shard.initiateTracking(request.targetAllocationId())); | ||
|
||
try { | ||
prepareTargetForTranslog(translog.estimateTotalOperationsFromMinSeq(startingSeqNo)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we can roll back all these naming changes if we we keep the old message (and the boolean) |
||
openEngineOnTarget(isSequenceNumberBasedRecoveryPossible, translog.estimateTotalOperationsFromMinSeq(startingSeqNo)); | ||
} catch (final Exception e) { | ||
throw new RecoveryEngineException(shard.shardId(), 1, "prepare target for translog failed", e); | ||
} | ||
|
@@ -421,13 +421,17 @@ public void phase1(final IndexCommit snapshot, final Supplier<Integer> translogO | |
} | ||
} | ||
|
||
void prepareTargetForTranslog(final int totalTranslogOps) throws IOException { | ||
void openEngineOnTarget(final boolean sequencedBasedRecovery, final int totalTranslogOps) throws IOException { | ||
StopWatch stopWatch = new StopWatch().start(); | ||
logger.trace("recovery [phase1]: prepare remote engine for translog"); | ||
final long startEngineStart = stopWatch.totalTime().millis(); | ||
// Send a request preparing the new shard's translog to receive operations. This ensures the shard engine is started and disables | ||
// garbage collection (not the JVM's GC!) of tombstone deletes. | ||
cancellableThreads.executeIO(() -> recoveryTarget.prepareForTranslogOperations(totalTranslogOps)); | ||
if (sequencedBasedRecovery) { | ||
cancellableThreads.executeIO(() -> recoveryTarget.openSequencedBasedEngine(totalTranslogOps)); | ||
} else { | ||
cancellableThreads.executeIO(() -> recoveryTarget.openFileBasedEngine(totalTranslogOps)); | ||
} | ||
stopWatch.stop(); | ||
|
||
response.startTime = stopWatch.totalTime().millis() - startEngineStart; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -362,12 +362,17 @@ private void ensureRefCount() { | |
/*** Implementation of {@link RecoveryTargetHandler } */ | ||
|
||
@Override | ||
public void prepareForTranslogOperations(int totalTranslogOps) throws IOException { | ||
public void openFileBasedEngine(int totalTranslogOps) throws IOException { | ||
state().getTranslog().totalOperations(totalTranslogOps); | ||
// TODO: take the local checkpoint from store as global checkpoint, once we know it's safe | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the todo is still relevant no? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I pushed it back as a note as it's a valid TODO. |
||
indexShard().openIndexAndCreateTranslog(false, SequenceNumbers.UNASSIGNED_SEQ_NO); | ||
} | ||
|
||
@Override | ||
public void openSequencedBasedEngine(int totalTranslogOps) throws IOException { | ||
state().getTranslog().totalOperations(totalTranslogOps); | ||
indexShard().openIndexAndSkipTranslogRecovery(); | ||
} | ||
|
||
@Override | ||
public void finalizeRecovery(final long globalCheckpoint) throws IOException { | ||
final IndexShard indexShard = indexShard(); | ||
|
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.
I've slept on this (as promised :)) and I prefer we go back to how you had it with a boolean in
RecoveryPrepareForTranslogOperationsRequest
. The reason is that I want to do some refactoring to simplify how the engine is created and I expect this to change making the boolean not needed and only use one message. I rather not have to deal with two messages and another layer of BWC. I think we should call the boolean "deleteLocalTranslog"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.
+1 on not having a separate message here.