Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rebuild version map when opening internal engine #9394

Merged
merged 2 commits into from Dec 4, 2019
Merged

Conversation

marregui
Copy link
Contributor

With this change, we will rebuild the live version map and local checkpoint using
documents (including soft-deleted) from the safe commit when opening an internal
engine. This allows us to safely prune away _id of all soft-deleted documents as
the version map is always in-sync with Lucene index.

Relates #40741 (elastic/elasticsearch#40741)
Supersedes #42979 (elastic/elasticsearch#42979)

Port of elastic/elasticsearch#43202

@marregui marregui self-assigned this Nov 28, 2019
@marregui marregui added the v/4.0 label Nov 28, 2019
@marregui marregui requested a review from seut November 28, 2019 16:05
@seut
Copy link
Member

seut commented Nov 28, 2019

@marregui
Copy link
Contributor Author

marregui commented Nov 28, 2019

That test uses a class that was renamed from DocIdSeqNoAndTerm to DocIdSeqNoAndSource in the context of this PR https://github.com/elastic/elasticsearch/pull/41661/files
I copied the test from ES 7.5 (and used DocIdSeqNoAndTerm) and when I run it I get


java.lang.AssertionError
	at __randomizedtesting.SeedInfo.seed([795A50AD16F3298D:8D446012A0902F4C]:0)
	at org.elasticsearch.index.engine.InternalEngine.assertMaxSeqNoOfUpdatesIsAdvanced(InternalEngine.java:2779)
	at org.elasticsearch.index.engine.InternalEngine.indexIntoLucene(InternalEngine.java:1100)
	at org.elasticsearch.index.engine.InternalEngine.index(InternalEngine.java:910)
	at org.elasticsearch.index.engine.TranslogHandler.applyOperation(TranslogHandler.java:75)
	at org.elasticsearch.index.engine.TranslogHandler.run(TranslogHandler.java:100)

I thought that at that point I would stop and reconsider.

@seut
Copy link
Member

seut commented Nov 29, 2019

@marregui So why not backport elastic/elasticsearch#41661 first?
I think the mentioned test is related as it will validate the changes made by this PR.

@marregui
Copy link
Contributor Author

marregui commented Nov 29, 2019

Thank you, I thought about it, and that is partially the reason I stopped, to get feedback.

PR #9399

marregui added a commit that referenced this pull request Nov 29, 2019
With this change, we will verify the consistency of version and source
(besides id, seq_no, and term) of live documents between shard copies
at the end of disruption tests.

Port of elastic/elasticsearch#41614
Required by #9394
mergify bot pushed a commit that referenced this pull request Nov 29, 2019
With this change, we will verify the consistency of version and source
(besides id, seq_no, and term) of live documents between shard copies
at the end of disruption tests.

Port of elastic/elasticsearch#41614
Required by #9394
mergify bot pushed a commit that referenced this pull request Nov 29, 2019
With this change, we will verify the consistency of version and source
(besides id, seq_no, and term) of live documents between shard copies
at the end of disruption tests.

Port of elastic/elasticsearch#41614
Required by #9394

(cherry picked from commit ddba3be)
mergify bot pushed a commit that referenced this pull request Nov 29, 2019
With this change, we will verify the consistency of version and source
(besides id, seq_no, and term) of live documents between shard copies
at the end of disruption tests.

Port of elastic/elasticsearch#41614
Required by #9394

(cherry picked from commit ddba3be)
@marregui
Copy link
Contributor Author

marregui commented Nov 29, 2019

I believe I need to port elastic/elasticsearch#41161 as well, test assertMaxSeqNoOfUpdatesIsAdvanced defined in class InternalEngine will fail in:

        if (maxSeqNoOfUpdates == SequenceNumbers.UNASSIGNED_SEQ_NO) {
            assert config().getIndexSettings().getIndexVersionCreated().before(Version.ES_V_6_5_1);
            return true;
        }

this code is removed in PR41161

@seut
Copy link
Member

seut commented Dec 2, 2019

@marregui Yes please backport elastic/elasticsearch#41161 upfront as well.

@marregui
Copy link
Contributor Author

marregui commented Dec 2, 2019

and that one requires this one: elastic/elasticsearch#39571 it is very simple.

With this change, we will rebuild the live version map and local checkpoint using
documents (including soft-deleted) from the safe commit when opening an internal
engine. This allows us to safely prune away _id of all soft-deleted documents as
the version map is always in-sync with Lucene index.

Relates #40741 (elastic/elasticsearch#40741)
Supersedes #42979 (elastic/elasticsearch#42979)

Port of elastic/elasticsearch#43202
Copy link
Member

@seut seut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@marregui marregui added the ready-to-merge Let Mergify merge the PR once approved and checks pass label Dec 4, 2019
@mergify mergify bot merged commit bb23cb7 into master Dec 4, 2019
@mergify mergify bot deleted the ma/flaky_test branch December 4, 2019 08:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge Let Mergify merge the PR once approved and checks pass v/4.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants