Remove LocalCheckpointTracker#resetCheckpoint #34667
Conversation
This commit removes resetCheckpoint from LocalCheckpoinTracker and rewrites testDedupByPrimaryTerm without resetting the local checkpoint.
dnhatn
added
>non-issue
v7.0.0
:Distributed Indexing/Engine
|
Pinging @elastic/es-distributed |
Should be fixed by elastic#34667
|
@dnhatn does that test even have value today? I think that we now rollback, we should never override a term in lucene? (actually we should never override an operation) |
|
@bleskes If an operation above the local checkpoint is delivered multiple times, we will index multiple copies into Lucene (please see https://github.com/elastic/elasticsearch/blob/master/server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java#L920). We can make the Engine to index once. WDYT? |
|
@bleskes I renamed and added a comment for the test. It's ready for another round. |
server/src/test/java/org/elasticsearch/index/engine/LuceneChangesSnapshotTests.java
Outdated
Show resolved
Hide resolved
| int totalOps = 0; | ||
| for (Engine.Operation op : operations) { | ||
| // Engine skips deletes or indexes below the local checkpoint | ||
| if (engine.getLocalCheckpoint() < op.seqNo() || op instanceof Engine.NoOp) { |
There was a problem hiding this comment.
The fact that we process noops differently than indexing / delete ops (w.r.t localcheckpoint) sounds like a bug (different) PR)
There was a problem hiding this comment.
sure, I will make this in a follow up.
I think this is a good thing to do and we discussed it in the context of the following engine. I think that to fully do it we need to have soft deletes and restore the local checkpoint tracker correctly when opening the engine. Based on that, I think we need to wait until soft deletes become a default. |
On a second thought, I think there is value in beta testing this before. I would need to see how it looks like to make a better judgment call. That said - no rush what so ever to do this. |
|
Thanks @bleskes. |
In #34474, we added a new assertion to ensure that the LocalCheckpointTracker is always consistent with Lucene index. However, we reset LocalCheckpoinTracker in testDedupByPrimaryTerm cause this assertion to be violated. This commit removes resetCheckpoint from LocalCheckpointTracker and rewrites testDedupByPrimaryTerm without resetting the local checkpoint. Relates #34474
* elastic/6.x: (37 commits) [HLRC] Added support for Follow Stats API (elastic#36253) Exposed engine must have all ops below gcp during rollback (elastic#36159) TEST: Always enable soft-deletes in ShardChangesTests Use delCount of SegmentInfos to calculate numDocs (elastic#36323) Add soft-deletes upgrade tests (elastic#36286) Remove LocalCheckpointTracker#resetCheckpoint (elastic#34667) Option to use endpoints starting with _security (elastic#36379) [CCR] Restructured QA modules (elastic#36404) RestClient: on retry timeout add root exception (elastic#25576) [HLRC] Add support for put privileges API (elastic#35679) HLRC: Add rollup search (elastic#36334) Explicitly recommend to forceMerge before freezing (elastic#36376) Rename internal repository actions to be internal (elastic#36377) Core: Remove parseDefaulting from DateFormatter (elastic#36386) [ML] Prevent stack overflow while copying ML jobs and datafeeds (elastic#36370) Docs: Fix Jackson reference (elastic#36366) [ILM] Fix issue where index may not yet be in 'hot' phase (elastic#35716) Undeprecate /_watcher endpoints (elastic#36269) Docs: Fix typo in bool query (elastic#36350) HLRC: Add delete template API (elastic#36320) ...
See upstream changes: - elastic/elasticsearch#38755 - elastic/elasticsearch#34667
See upstream changes: - elastic/elasticsearch#38755 - elastic/elasticsearch#34667
See upstream changes: - elastic/elasticsearch#38755 - elastic/elasticsearch#34667
In #34474, we added a new assertion to ensure that the LocalCheckpointTracker is always consistent with Lucene index. However, we reset LocalCheckpoinTracker in testDedupByPrimaryTerm cause this assertion to be violated.
This commit removes resetCheckpoint from LocalCheckpointTracker and rewrites testDedupByPrimaryTerm without resetting the local checkpoint.