Preserve multiple translog generations #24015
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -85,14 +85,14 @@ | |
| * When a translog is opened the checkpoint is use to retrieve the latest translog file generation and subsequently to open the last written file to recovery operations. | ||
| * The {@link org.elasticsearch.index.translog.Translog.TranslogGeneration}, given when the translog is opened / constructed is compared against | ||
| * the latest generation and all consecutive translog files singe the given generation and the last generation in the checkpoint will be recovered and preserved until the next | ||
| * generation is committed using {@link Translog#commit(long)}. In the common case the translog file generation in the checkpoint and the generation passed to the translog on creation are | ||
| * the same. The only situation when they can be different is when an actual translog commit fails in between {@link Translog#prepareCommit()} and {@link Translog#commit(long)}. In such a case | ||
| * the currently being committed translog file will not be deleted since it's commit was not successful. Yet, a new/current translog file is already opened at that point such that there is more than | ||
| * one translog file present. Such an uncommitted translog file always has a <tt>translog-${gen}.ckp</tt> associated with it which is an fsynced copy of the it's last <tt>translog.ckp</tt> such that in | ||
| * disaster recovery last fsynced offsets, number of operation etc. are still preserved. | ||
| * </p> | ||
| */ | ||
| public class Translog extends AbstractIndexShardComponent implements IndexShardComponent, Closeable { | ||
|
|
||
| /* | ||
| * TODO | ||
|
|
@@ -1347,6 +1347,28 @@ public static void writeOperationNoSize(BufferedChecksumStreamOutput out, Transl | |
| out.writeInt((int) checksum); | ||
| } | ||
|
|
||
| /** | ||
| * Gets the minimum generation that could contain the sequence number, or the current generation if there is no generation with the | ||
| * specified sequence number between the minimum and maximum sequence numbers. | ||
| * | ||
| * @param seqNo the sequence number | ||
| * @return the minimum generation for the sequence number, or the current generation | ||
| */ | ||
| public TranslogGeneration getMinGenerationForSeqNo(final long seqNo) { | ||
| try (ReleasableLock ignored = writeLock.acquire()) { | ||
| final long minTranslogFileGeneration = readers | ||
|
There was a problem hiding this comment. I wonder if we can just use a for loop and long minTranslogFileGeneration = this.currentFileGeneration;
for (Reader r : readers) {
if (seqNo <= r.getCheckpoint().maxSeqId) {
minTranslogFileGeneration = Math.min(minTranslogFileGeneration, r. getGeneration());
}
} |
||
| .stream() | ||
| .filter(r -> { | ||
| final Checkpoint checkpoint = r.getCheckpoint(); | ||
| return checkpoint.minSeqNo <= seqNo && seqNo <= checkpoint.maxSeqNo; | ||
|
There was a problem hiding this comment. hmm... why do we need to check the lower bound? isn't it simpler to check There was a problem hiding this comment. I think that was addressed by c63543b which I think I pushed while you were reviewing. |
||
| }) | ||
| .mapToLong(TranslogReader::getGeneration) | ||
| .min() | ||
| .orElseGet(this::currentFileGeneration); | ||
|
There was a problem hiding this comment. hmm. When is this or else actually triggers? I guess it can only happens if you potentially have bwc readers and and one or more empty current ones. Should we assert for this? There was a problem hiding this comment. It can happen because we commit when we open the translog. There was a problem hiding this comment. clear. This is maybe worth a comment? There was a problem hiding this comment. I think another point is that when we are flushing the engine, we ask the translog for the minimum generation for the local checkpoint plus one which might not yet be in the translog. There was a problem hiding this comment. That's another very good point |
||
| return new TranslogGeneration(translogUUID, minTranslogFileGeneration); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Roll the current translog generation into a new generation. This does not commit the | ||
| * translog. | ||
|
|
@@ -1375,25 +1397,35 @@ public void rollGeneration() throws IOException { | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * Prepares a translog commit by setting the current committing generation and rolling the translog generation. | ||
| * | ||
| * @throws IOException if an I/O exception occurred while rolling the translog generation | ||
| */ | ||
| public void prepareCommit() throws IOException { | ||
|
There was a problem hiding this comment. now that we don't use the 2phase commit interface anymore we can rename these methods IMO There was a problem hiding this comment. I agree, let's do this in a small PR that immediately follows this PR. |
||
| try (ReleasableLock ignored = writeLock.acquire()) { | ||
| ensureOpen(); | ||
| if (currentCommittingGeneration != NOT_SET_GENERATION) { | ||
| final String message = | ||
| String.format(Locale.ROOT, "already committing a translog with generation [%d]", currentCommittingGeneration); | ||
| throw new IllegalStateException(message); | ||
| } | ||
| currentCommittingGeneration = current.getGeneration(); | ||
| rollGeneration(); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Commits the translog and sets the last committed translog generation to the specified generation. The specified committed generation | ||
| * will be used when trimming unreferenced translog generations such that generations from the committed generation will be preserved. | ||
| * | ||
| * If {@link Translog#prepareCommit()} was not called before calling commit, this method will be invoked too causing the translog | ||
| * generation to be rolled. | ||
| * | ||
| * @param committedGeneration the minimum translog generation to preserve after trimming unreferenced generations | ||
| * @throws IOException if an I/O exception occurred preparing the translog commit | ||
| */ | ||
| public void commit(final long committedGeneration) throws IOException { | ||
|
There was a problem hiding this comment. I think this parameter name is confusing given that we have There was a problem hiding this comment. Let's focus on naming in a folow-up PR. |
||
| try (ReleasableLock ignored = writeLock.acquire()) { | ||
| ensureOpen(); | ||
| if (currentCommittingGeneration == NOT_SET_GENERATION) { | ||
|
There was a problem hiding this comment. I'm not sure this makes sense anymore? should we just forbid it with an exception? or better yet require that the generation given to use is <= current and be done with it? There was a problem hiding this comment. Are you saying that we should require preparing a commit (invoking |
||
|
|
@@ -1403,26 +1435,30 @@ public long commit() throws IOException { | |
| assert readers.stream().anyMatch(r -> r.getGeneration() == currentCommittingGeneration) | ||
| : "readers missing committing generation [" + currentCommittingGeneration + "]"; | ||
| // set the last committed generation otherwise old files will not be cleaned up | ||
| lastCommittedTranslogFileGeneration = committedGeneration; | ||
| currentCommittingGeneration = NOT_SET_GENERATION; | ||
| trimUnreferencedReaders(); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Trims unreferenced translog generations. The guarantee here is that translog generations will be preserved for all outstanding views | ||
| * and from the last committed translog generation defined by {@link Translog#lastCommittedTranslogFileGeneration}. | ||
| */ | ||
| void trimUnreferencedReaders() { | ||
| try (ReleasableLock ignored = writeLock.acquire()) { | ||
| if (closed.get()) { | ||
| // we're shutdown potentially on some tragic event, don't delete anything | ||
| return; | ||
| } | ||
| long minReferencedGen = Math.min( | ||
|
There was a problem hiding this comment. did something change here that is related it's hard to tell maybe we can revert this unnecessary modification? There was a problem hiding this comment. I see a small improvement here which is removing the need for the extra local variable |
||
| lastCommittedTranslogFileGeneration, | ||
| outstandingViews.stream().mapToLong(View::minTranslogGeneration).min().orElse(Long.MAX_VALUE)); | ||
| final List<TranslogReader> unreferenced = | ||
| readers.stream().filter(r -> r.getGeneration() < minReferencedGen).collect(Collectors.toList()); | ||
| for (final TranslogReader unreferencedReader : unreferenced) { | ||
| final Path translogPath = unreferencedReader.path(); | ||
| logger.trace("delete translog file [{}], not referenced and not current anymore", translogPath); | ||
| IOUtils.closeWhileHandlingException(unreferencedReader); | ||
| IOUtils.deleteFilesIgnoringExceptions(translogPath, | ||
| translogPath.resolveSibling(getCommitCheckpointFileName(unreferencedReader.getGeneration()))); | ||
|
|
@@ -1442,13 +1478,6 @@ void closeFilesIfNoPendingViews() throws IOException { | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * References a transaction log generation | ||
| */ | ||
|
|
||
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 so wish we could come up with a better name for this one - I find it confusing every time. How about
translogGenForRecovery?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 don't understand this comment, this is a new local variable that was never there before. Did you maybe mean to comment on
Translog#lastCommittedTranslogFileGenerationwhich we have discussed the confusing nature of before?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 think all of them suffer from the same problem. This is just the first one I run into while reviewing. I think I get why it's named like this now (the translog id that was committed into lucene's metadata) but there is much room for ambiguity - is the the maximum generation for which all ops are in lucene (i.e., committed)? I wonder every time and end up double checking the code. If people find this name better, I'm good with keeping it - it's a subjective matter.
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.
One naming thing that gets to me is
prepareCommitandcommitinTranslog. I don't thinkcommitcommits anything. It trims segments based on a commit in permanent storage. I wonder if it'd make the naming easier to stop using the word "commit" for the thing that the Translog is doing. Then any time you see the word "commit" it is about a lucene commit. If you renamedTranslog#commitintoTranslog#updatePersistedGenerationor something then it'd be more clear that it is reacting to a commit in Lucene. Then maybe the other variable names just fall out of that?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 would like to focus on the naming in a small follow-up PR immediately after this PR.