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

Fix FollowingEngineTests#testOptimizeMultipleVersions #75583

Merged
merged 6 commits into from
Sep 2, 2021

Conversation

fcofdez
Copy link
Contributor

@fcofdez fcofdez commented Jul 21, 2021

In certain concurrent indexing scenarios where there are deletes
executed and then a new indexing operation, the following engine
considers those as updates breaking one of the assumed invariants.

Closes #72527

In certain concurrent indexing scenarios where there are deletes
executed and then a new indexing operation, the following engine
considers those as updates breaking one of the assumed invariants.

Closes elastic#72527
@fcofdez fcofdez added >test Issues or PRs that are addressing/adding tests :Distributed Indexing/CCR Issues around the Cross Cluster State Replication features v8.0.0 Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. v7.15.0 labels Jul 21, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed (Team:Distributed)

@henningandersen henningandersen self-requested a review August 5, 2021 08:01
Copy link
Contributor

@henningandersen henningandersen left a comment

Choose a reason for hiding this comment

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

I left a few comments, mainly on the test.

@fcofdez
Copy link
Contributor Author

fcofdez commented Aug 31, 2021

Sorry, I didn't have the chance to look into this until now. I've added a synchronization point between the test threads that should make the issue easier to reproduce, before it took > 1000 runs to reproduce.

Copy link
Contributor

@henningandersen henningandersen left a comment

Choose a reason for hiding this comment

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

LGTM.

@@ -118,14 +118,27 @@ protected long generateSeqNoForOperationOnPrimary(final Operation operation) {
}

@Override
protected void advanceMaxSeqNoOfUpdatesOrDeletesOnPrimary(long seqNo) {
protected void advanceMaxSeqNoOfDeleteOnPrimary(long seqNo) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should keep this in plural, i.e., advanceMaxSeqNoOfDeletesOnPrimary

}

@Override
protected void advanceMaxSeqNoOfUpdateOnPrimary(long seqNo) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Also plural here for consistency.

// maxSeqNoOfUpdatesOrDeletes, since in this engine (effectively it is a replica) we don't check if the previous version
// was a delete and it's possible to consider it as an update, advancing the max sequence number over the leader
// maxSeqNoOfUpdatesOrDeletes.
// The goal of this marker it's just an optimization and it won't affect the correctness or durability of the indexed documents.
Copy link
Contributor

Choose a reason for hiding this comment

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

A wrong number could lead to incorrect behavior so I suggest to rephrase this to something like:
// We conservatively advance the seqno in this case, accepting a minor performance hit in this edge case.

@fcofdez
Copy link
Contributor Author

fcofdez commented Sep 2, 2021

@elasticmachine update branch

@fcofdez fcofdez added the auto-backport Automatically create backport pull requests when merged label Sep 2, 2021
@fcofdez fcofdez merged commit b7838d6 into elastic:master Sep 2, 2021
fcofdez added a commit to fcofdez/elasticsearch that referenced this pull request Sep 2, 2021
In certain concurrent indexing scenarios where there are deletes
executed and then a new indexing operation, the following engine
considers those as updates breaking one of the assumed invariants.

Closes elastic#72527
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
7.x

fcofdez added a commit that referenced this pull request Sep 2, 2021
In certain concurrent indexing scenarios where there are deletes
executed and then a new indexing operation, the following engine
considers those as updates breaking one of the assumed invariants.

Closes #72527
Backport of #75583
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Automatically create backport pull requests when merged :Distributed Indexing/CCR Issues around the Cross Cluster State Replication features Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. >test Issues or PRs that are addressing/adding tests v7.16.0 v8.0.0-alpha2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CI] FollowingEngineTests testOptimizeMultipleVersions failing
6 participants