Advance max_seq_no before add operation to Lucene #38879
Merged
Conversation
This file contains 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
dnhatn
added
:Distributed Indexing/Engine
|
Pinging @elastic/es-distributed |
ywelsch
suggested changes
Feb 14, 2019
| @@ -868,6 +868,7 @@ public IndexResult index(Index index) throws IOException { | |||
| indexResult = plan.earlyResultOnPreFlightError.get(); | |||
| assert indexResult.getResultType() == Result.Type.FAILURE : indexResult.getResultType(); | |||
| } else if (plan.indexIntoLucene || plan.addStaleOpToLucene) { | |||
| localCheckpointTracker.advanceMaxSeqNo(plan.seqNoForIndexing); | |||
There was a problem hiding this comment.
Properly setting the max sequence number before indexing might not only be a useful property for Lucene-indexed stuff, but in general. I would prefer not to hide it in this sub-condition.
It bugs me that the planning process is mutating the state of the LocalCheckpointTracker in this way when being primary, but not when being replica. If we want the flow for the primary and replica to be the same, we could do this instead as part of the planning process in planIndexingAsNonPrimary. This will make it so that both primary and replica manage this property in the planning phase (will also require adaptation to FollowingEngine).
As a follow-up, I think we should also investigate whether we can remove this state mutation from the planning process, i.e., not have the planning process assign a sequence number or change the max seq no, but do it as an explicit step here in the index method, given that it is such a fundamental step in processing a request.
| try (DirectoryReader reader = DirectoryReader.open(commit)) { | ||
| AtomicLong maxSeqNoFromDocs = new AtomicLong(SequenceNumbers.NO_OPS_PERFORMED); | ||
| Lucene.scanSeqNosInReader(reader, 0, maxNumDocs, n -> maxSeqNoFromDocs.set(Math.max(n, maxSeqNoFromDocs.get()))); | ||
| assertThat(Long.parseLong(commit.getUserData().get(SequenceNumbers.MAX_SEQ_NO)), |
There was a problem hiding this comment.
can we check this property across all of our Engine tests?
|
@ywelsch it's ready again. Can you have another look? Thank you! |
|
I believe the optimization using sequence numbers on the FollowingEngine has hit this bug in #38894. |
|
@ywelsch Thanks for reviewing. |
dnhatn
added a commit
that referenced
this pull request
Feb 15, 2019
Today when processing an operation on a replica engine (or the following engine), we first add it to Lucene, then add it to translog, then finally marks its seq_no as completed. If a flush occurs after step1, but before step-3, the max_seq_no in the commit's user_data will be smaller than the seq_no of some documents in the Lucene commit.
jasontedor
added a commit
to jasontedor/elasticsearch
that referenced
this pull request
Feb 15, 2019
* master: Address some CCR REST test case flakiness (elastic#38975) Edits to text in Completion Suggester doc (elastic#38980) SQL: doc polishing [DOCS] Fixes broken formatting SQL: Polish the rest chapter (elastic#38971) Remove `nGram` and `edgeNGram` token filter names (elastic#38911) Add an exception throw if waiting on transport port file fails (elastic#37574) Improve testcluster distribution artifact handling (elastic#38933) Advance max_seq_no before add operation to Lucene (elastic#38879) Reduce global checkpoint sync interval in disruption tests (elastic#38931) [test] disable packaging tests for suse boxes Relax testStressMaybeFlushOrRollTranslogGeneration (elastic#38918) [DOCS] Edits warning in put watch API (elastic#38582) Fix serialization bug in ShardFollowTask after cutting this class over to extend from ImmutableFollowParameters. [DOCS] Updates methods for upgrading machine learning (elastic#38876)
dnhatn
added a commit
that referenced
this pull request
Feb 15, 2019
Today when processing an operation on a replica engine (or the following engine), we first add it to Lucene, then add it to translog, then finally marks its seq_no as completed. If a flush occurs after step1, but before step-3, the max_seq_no in the commit's user_data will be smaller than the seq_no of some documents in the Lucene commit.
dnhatn
added a commit
that referenced
this pull request
Feb 16, 2019
Today when processing an operation on a replica engine (or the following engine), we first add it to Lucene, then add it to translog, then finally marks its seq_no as completed. If a flush occurs after step1, but before step-3, the max_seq_no in the commit's user_data will be smaller than the seq_no of some documents in the Lucene commit.
dnhatn
added a commit
that referenced
this pull request
Feb 16, 2019
Today when processing an operation on a replica engine (or the following engine), we first add it to Lucene, then add it to translog, then finally marks its seq_no as completed. If a flush occurs after step1, but before step-3, the max_seq_no in the commit's user_data will be smaller than the seq_no of some documents in the Lucene commit.
dnhatn
added a commit
that referenced
this pull request
Sep 7, 2019
The max_seq_no of Lucene commit of the old indices (before 6.6.2) can be smaller than seq_no of some documents in the commit (see #38879). Although we fixed this bug in 6.6.2 and 7.0.0, a problematic index commit can still affect the newer version after a rolling upgrade or full cluster restart. In particular, if a FollowingEngine (or an internal engine with MSU enabled) restores from a problematic commit, then it can apply MSU optimization for existing documents. The symptom that we see here is the local checkpoint tracker assertion is violated. Closes #46311 Relates #38879
marregui
added a commit
to crate/crate
that referenced
this pull request
Nov 18, 2019
This commit fixes the issue of potential mismatches between the max_seq_no in the commit's user_data and the seq_no of some documents in the Lucene commit. The mismatch could arise when processing an operation on a replica engine, as we first added it to Lucene, then to the translog, to finally mark seq_no as completed. If a flush occurred after step1, but before the marking, then the max_seq_no in the commit's user_data would be smaller than the seq_no of some documents in the Lucene commit. Port of elastic/elasticsearch#38879
mergify bot
pushed a commit
to crate/crate
that referenced
this pull request
Nov 18, 2019
This commit fixes the issue of potential mismatches between the max_seq_no in the commit's user_data and the seq_no of some documents in the Lucene commit. The mismatch could arise when processing an operation on a replica engine, as we first added it to Lucene, then to the translog, to finally mark seq_no as completed. If a flush occurred after step1, but before the marking, then the max_seq_no in the commit's user_data would be smaller than the seq_no of some documents in the Lucene commit. Port of elastic/elasticsearch#38879
marregui
added a commit
to crate/crate
that referenced
this pull request
Nov 18, 2019
Advance max_seq_no before add operation to Lucene. This commit fixes the issue of potential mismatches between the max_seq_no in the commit's user_data and the seq_no of some documents in the Lucene commit. The mismatch could arise when processing an operation on a replica engine, as we first added it to Lucene, then to the translog, to finally mark seq_no as completed. If a flush occurred after step1, but before the marking, then the max_seq_no in the commit's user_data would be smaller than the seq_no of some documents in the Lucene commit. Port of elastic/elasticsearch#38879
marregui
added a commit
to crate/crate
that referenced
this pull request
Nov 18, 2019
This commit fixes the issue of potential mismatches between the max_seq_no in the commit's user_data and the seq_no of some documents in the Lucene commit. The mismatch could arise when processing an operation on a replica engine, as we first added it to Lucene, then to the translog, to finally mark seq_no as completed. If a flush occurred after step1, but before the marking, then the max_seq_no in the commit's user_data would be smaller than the seq_no of some documents in the Lucene commit. Port of elastic/elasticsearch#38879
mergify bot
pushed a commit
to crate/crate
that referenced
this pull request
Nov 18, 2019
This commit fixes the issue of potential mismatches between the max_seq_no in the commit's user_data and the seq_no of some documents in the Lucene commit. The mismatch could arise when processing an operation on a replica engine, as we first added it to Lucene, then to the translog, to finally mark seq_no as completed. If a flush occurred after step1, but before the marking, then the max_seq_no in the commit's user_data would be smaller than the seq_no of some documents in the Lucene commit. Port of elastic/elasticsearch#38879
mergify bot
pushed a commit
to crate/crate
that referenced
this pull request
Dec 4, 2019
This commit fixes the issue of potential mismatches between the max_seq_no in the commit's user_data and the seq_no of some documents in the Lucene commit. The mismatch could arise when processing an operation on a replica engine, as we first added it to Lucene, then to the translog, to finally mark seq_no as completed. If a flush occurred after step1, but before the marking, then the max_seq_no in the commit's user_data would be smaller than the seq_no of some documents in the Lucene commit. Port of elastic/elasticsearch#38879 (cherry picked from commit b126ec5) # Conflicts: # es/es-server/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java # es/es-testing/src/main/java/org/elasticsearch/index/engine/EngineTestCase.java
mergify bot
pushed a commit
to crate/crate
that referenced
this pull request
Dec 4, 2019
This commit fixes the issue of potential mismatches between the max_seq_no in the commit's user_data and the seq_no of some documents in the Lucene commit. The mismatch could arise when processing an operation on a replica engine, as we first added it to Lucene, then to the translog, to finally mark seq_no as completed. If a flush occurred after step1, but before the marking, then the max_seq_no in the commit's user_data would be smaller than the seq_no of some documents in the Lucene commit. Port of elastic/elasticsearch#38879 (cherry picked from commit 505cdf8) # Conflicts: # es/es-testing/src/main/java/org/elasticsearch/index/engine/EngineTestCase.java
marregui
added a commit
to crate/crate
that referenced
this pull request
Dec 5, 2019
This commit fixes the issue of potential mismatches between the max_seq_no in the commit's user_data and the seq_no of some documents in the Lucene commit. The mismatch could arise when processing an operation on a replica engine, as we first added it to Lucene, then to the translog, to finally mark seq_no as completed. If a flush occurred after step1, but before the marking, then the max_seq_no in the commit's user_data would be smaller than the seq_no of some documents in the Lucene commit. Port of elastic/elasticsearch#38879 (cherry picked from commit b126ec5)
mergify bot
pushed a commit
to crate/crate
that referenced
this pull request
Dec 5, 2019
This commit fixes the issue of potential mismatches between the max_seq_no in the commit's user_data and the seq_no of some documents in the Lucene commit. The mismatch could arise when processing an operation on a replica engine, as we first added it to Lucene, then to the translog, to finally mark seq_no as completed. If a flush occurred after step1, but before the marking, then the max_seq_no in the commit's user_data would be smaller than the seq_no of some documents in the Lucene commit. Port of elastic/elasticsearch#38879 (cherry picked from commit b126ec5)
marregui
added a commit
to crate/crate
that referenced
this pull request
Dec 5, 2019
This commit fixes the issue of potential mismatches between the max_seq_no in the commit's user_data and the seq_no of some documents in the Lucene commit. The mismatch could arise when processing an operation on a replica engine, as we first added it to Lucene, then to the translog, to finally mark seq_no as completed. If a flush occurred after step1, but before the marking, then the max_seq_no in the commit's user_data would be smaller than the seq_no of some documents in the Lucene commit. Port of elastic/elasticsearch#38879 (cherry picked from commit 505cdf8)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 when processing an operation on a replica engine (or the following engine), we first add it to Lucene, then add it to translog, then finally marks its seq_no as completed. If a flush occurs after step1, but before step3, the max_seq_no in the commit's user_data will be smaller than the seq_no of some documents in the Lucene commit.
I found this issue while investigating the test failure in #31629.