Backport for using lastSyncedGlobalCheckpoint in deletion policy#27866
Merged
dnhatn merged 7 commits intoelastic:6.xfrom Dec 19, 2017
Merged
Backport for using lastSyncedGlobalCheckpoint in deletion policy#27866dnhatn merged 7 commits intoelastic:6.xfrom
dnhatn merged 7 commits intoelastic:6.xfrom
Conversation
Today we use the in-memory global checkpoint from SequenceNumbersService to clean up unneeded commit points, however the latest global checkpoint may haven't fsynced to the disk yet. If the translog checkpoint fsync failed and we already use a higher global checkpoint to clean up commit points, then we may have removed a safe commit which we try to keep for recovery. This commit updates the deletion policy using lastSyncedGlobalCheckpoint from Translog rather the in memory global checkpoint. Relates elastic#27606
ywelsch
reviewed
Dec 18, 2017
| private void refreshTranslogUUIDIfNeeded(IndexWriter writer, Translog translog) throws IOException { | ||
| final String translogUUID = commitDataAsMap(writer).get(Translog.TRANSLOG_UUID_KEY); | ||
| if (translog.getTranslogUUID().equals(translogUUID) == false) { | ||
| assert openMode != EngineConfig.OpenMode.OPEN_INDEX_AND_TRANSLOG : "OPEN_INDEX_AND_TRANSLOG must not fresh translogUUID"; |
Contributor
There was a problem hiding this comment.
assertion message reads funny
| assert translog.getGeneration() != null; | ||
| // we can only do this after we generated and committed a translog uuid. other wise the combined | ||
| // retention policy, which listens to commits, gets all confused. | ||
| refreshTranslogUUIDIfNeeded(writer, translog); |
Contributor
There was a problem hiding this comment.
move this line above the comment? The comment applies to persistHistoryUUIDIfNeeded
bleskes
reviewed
Dec 18, 2017
| assert translog.getGeneration() != null; | ||
| // we can only do this after we generated and committed a translog uuid. other wise the combined | ||
| // retention policy, which listens to commits, gets all confused. | ||
| refreshTranslogUUIDIfNeeded(writer, translog); |
Contributor
There was a problem hiding this comment.
I prefer it like this, wdyt?
diff --git a/core/src/main/java/org/elasticsearch/index/engine/InternalEngine.java b/core/src/main/java/org/elasticsearch/index/engine/InternalEngine.java
index b0523a1c3d2..59ddbb82ccb 100644
--- a/core/src/main/java/org/elasticsearch/index/engine/InternalEngine.java
+++ b/core/src/main/java/org/elasticsearch/index/engine/InternalEngine.java
@@ -198,10 +198,11 @@ public class InternalEngine extends Engine {
historyUUID = loadOrGenerateHistoryUUID(writer, engineConfig.getForceNewHistoryUUID());
Objects.requireNonNull(historyUUID, "history uuid should not be null");
indexWriter = writer;
- // we can only do this after we generated and committed a translog uuid. other wise the combined
- // retention policy, which listens to commits, gets all confused.
- refreshTranslogUUIDIfNeeded(writer, translog);
- persistHistoryUUIDIfNeeded();
+ if (openMode == EngineConfig.OpenMode.OPEN_INDEX_AND_TRANSLOG) {
+ persistHistoryUUIDIfNeeded();
+ } else {
+ commitNewTranslogAndHistoryUUIDs(writer, translog);
+ }
} catch (IOException | TranslogCorruptedException e) {
throw new EngineCreationFailureException(shardId, "failed to create engine", e);
} catch (AssertionError e) {
@@ -461,13 +462,15 @@ public class InternalEngine extends Engine {
return new Translog(translogConfig, translogUUID, translogDeletionPolicy, globalCheckpointSupplier);
}
- private void refreshTranslogUUIDIfNeeded(IndexWriter writer, Translog translog) throws IOException {
- final String translogUUID = commitDataAsMap(writer).get(Translog.TRANSLOG_UUID_KEY);
- if (translog.getTranslogUUID().equals(translogUUID) == false) {
- assert openMode != EngineConfig.OpenMode.OPEN_INDEX_AND_TRANSLOG : "OPEN_INDEX_AND_TRANSLOG must not fresh translogUUID";
- commitIndexWriter(writer, translog, openMode == EngineConfig.OpenMode.OPEN_INDEX_CREATE_TRANSLOG
- ? commitDataAsMap(writer).get(SYNC_COMMIT_ID) : null);
- }
+ private void commitNewTranslogAndHistoryUUIDs(IndexWriter writer, Translog translog) throws IOException {
+ final Map<String, String> commitUserData = commitDataAsMap(writer);
+ assert commitUserData.get(Translog.TRANSLOG_UUID_KEY).equals(translog.getTranslogUUID()) == false :
+ "committing a new translog uuid but it's already equal (uuid [" + translog.getTranslogUUID() + "](";
+ assert openMode != EngineConfig.OpenMode.OPEN_INDEX_AND_TRANSLOG : "OPEN_INDEX_AND_TRANSLOG must not fresh translogUUID";
+ assert historyUUID.equals(commitUserData.containsKey(HISTORY_UUID_KEY)) == false || config().getForceNewHistoryUUID() == false :
+ "a new history id was forced, but the one in the commit is equal";
+ commitIndexWriter(writer, translog, openMode == EngineConfig.OpenMode.OPEN_INDEX_CREATE_TRANSLOG
+ ? commitUserData.get(SYNC_COMMIT_ID) : null);
}
Member
Author
|
please retest this. |
Member
Author
|
@ywelsch, This backport is tricky than we thought. Sorry for blocking your PR. |
Contributor
|
once the ci is green, this can go in. |
Member
Author
|
Thanks @bleskes for reviewing. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a backport of #27826.
@bleskes, Could you please take a quick look to make sure that this backport fit with your fix in #26734.