Use lastSyncedGlobalCheckpoint in deletion policy#27826
Merged
dnhatn merged 6 commits intoelastic:masterfrom Dec 16, 2017
Merged
Use lastSyncedGlobalCheckpoint in deletion policy#27826dnhatn merged 6 commits intoelastic:masterfrom
dnhatn merged 6 commits intoelastic:masterfrom
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.
bleskes
suggested changes
Dec 15, 2017
Contributor
bleskes
left a comment
There was a problem hiding this comment.
Thx Nhat. I left some comments
| Channels.readFromFileChannelWithEofException(channel, position, targetBuffer); | ||
| } | ||
|
|
||
| private static Checkpoint writeCheckpoint( |
Contributor
There was a problem hiding this comment.
why is this relevant? I rather not touch files that aren't relevant for the PR (feel free to push such a change as removal of dead code without a PR)
| globalCheckpoint.set(randomIntBetween( | ||
| Math.toIntExact(engine.seqNoService().getGlobalCheckpoint()), | ||
| Math.toIntExact(engine.seqNoService().getLocalCheckpoint()))); | ||
| engine = new InternalEngine(config(indexSettings, store, createTempDir(), NoMergePolicy.INSTANCE, null), seqNoServiceSupplier) { |
Contributor
There was a problem hiding this comment.
why did we lose the try with resources clause?
Member
Author
There was a problem hiding this comment.
We close the engine in tearDown but I put the try-clause back.
Contributor
There was a problem hiding this comment.
I see - we reuse the same engine variable. I think it's cleaner to have it contained.
| engine.getTranslog().sync(); | ||
| } | ||
| if (frequently()) { | ||
| final long lastSyncedGlobalCheckpoint = engine.getTranslog().getLastSyncedGlobalCheckpoint(); |
Contributor
There was a problem hiding this comment.
strictly speaking I think we need to read this from disk after the flush - i.e., make sure that what's on disk is OK.
| @Override | ||
| protected void commitIndexWriter(IndexWriter writer, Translog translog, String syncId) throws IOException { | ||
| // The global checkpoint is advanced but not fsynced yet. | ||
| final long lagging = seqNoService().getLocalCheckpoint() - seqNoService().getGlobalCheckpoint(); |
Contributor
There was a problem hiding this comment.
I don't follow this - why do we need to query the global checkpoint from the seqNoService? we alread have access to globalCheckpoint ?
I think what you mean here is something like this - no if no rarely:
// Advance the global checkpoint during the flush to create a lag between what's persisted in the translog (and is visible for CombinedDeletionPolicy) and what's in memory in the SequenceServices
globalCheckpoint.set(randomLongBetween(globalCheckpoint.get(), seqNoService().getLocalCheckpoint()));
wdyt?
Member
Author
|
@bleskes I have addressed your comments. Would you please take a look? Thank you. |
bleskes
approved these changes
Dec 16, 2017
Member
Author
|
Thanks @bleskes. |
dnhatn
added a commit
to dnhatn/elasticsearch
that referenced
this pull request
Dec 18, 2017
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
dnhatn
added a commit
that referenced
this pull request
Dec 19, 2017
) 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. This is a backport of #27826.
Member
Author
|
Backported in #27866 |
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.
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 #27606