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

Enforce retention leases require soft deletes #39922

Merged
merged 5 commits into from Mar 12, 2019

Conversation

dnhatn
Copy link
Member

@dnhatn dnhatn commented Mar 11, 2019

If a primary on 6.7 and a replica on 5.6 are running more than 5 minutes (retention leases background sync interval), the retention leases background sync will be triggered, and it will trip 6.7 node due to the illegal checkpoint value. We can fix the problem by making the returned checkpoint depend on the node version. This PR, however, chooses to enforce retention leases require soft deletes, and make retention-leases sync noop if soft deletes is disabled instead.

Closes #39914

I tried but failed to come up with a useful test. Any suggestions are welcome!

@dnhatn dnhatn added >bug v7.0.0 :Distributed/Distributed A catch all label for anything in the Distributed Area. If you aren't sure, use this one. v6.7.0 v8.0.0 v7.2.0 labels Mar 11, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

Copy link
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

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

The production code looks good. I left some suggestions for testing.

@dnhatn
Copy link
Member Author

dnhatn commented Mar 11, 2019

@jasontedor Thanks for looking. I pushed changes. Can you have another look please?

@dnhatn dnhatn requested a review from jasontedor March 11, 2019 21:09
@dnhatn dnhatn added the blocker label Mar 11, 2019
Copy link
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

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

LGTM.

@dnhatn
Copy link
Member Author

dnhatn commented Mar 12, 2019

@jasontedor thanks!

@dnhatn dnhatn merged commit 695b20f into elastic:master Mar 12, 2019
@dnhatn dnhatn deleted the lease-require-soft-deletes branch March 12, 2019 00:51
dnhatn added a commit that referenced this pull request Mar 12, 2019
If a primary on 6.7 and a replica on 5.6 are running more than 5 minutes
(retention leases background sync interval), the retention leases
background sync will be triggered, and it will trip 6.7 node due to the
illegal checkpoint value. We can fix the problem by making the returned
checkpoint depends on the node version. This PR, however, chooses to
enforce retention leases require soft deletes, and make retention leases
sync noop if soft deletes is disabled instead.

Closes #39914
dnhatn added a commit that referenced this pull request Mar 12, 2019
@jasontedor
Copy link
Member

Thanks @dnhatn. Great change.

dnhatn added a commit that referenced this pull request Mar 12, 2019
dnhatn added a commit that referenced this pull request Mar 12, 2019
If a primary on 6.7 and a replica on 5.6 are running more than 5 minutes
(retention leases background sync interval), the retention leases
background sync will be triggered, and it will trip 6.7 node due to the
illegal checkpoint value. We can fix the problem by making the returned
checkpoint depends on the node version. This PR, however, chooses to
enforce retention leases require soft deletes, and make retention leases
sync noop if soft deletes is disabled instead.

Closes #39914
dnhatn added a commit that referenced this pull request Mar 12, 2019
dnhatn added a commit that referenced this pull request Mar 12, 2019
If a primary on 6.7 and a replica on 5.6 are running more than 5 minutes
(retention leases background sync interval), the retention leases
background sync will be triggered, and it will trip 6.7 node due to the
illegal checkpoint value. We can fix the problem by making the returned
checkpoint depends on the node version. This PR, however, chooses to
enforce retention leases require soft deletes, and make retention leases
sync noop if soft deletes is disabled instead.

Closes #39914
polyfractal pushed a commit to polyfractal/elasticsearch that referenced this pull request Mar 19, 2019
If a primary on 6.7 and a replica on 5.6 are running more than 5 minutes
(retention leases background sync interval), the retention leases
background sync will be triggered, and it will trip 6.7 node due to the
illegal checkpoint value. We can fix the problem by making the returned
checkpoint depends on the node version. This PR, however, chooses to
enforce retention leases require soft deletes, and make retention leases
sync noop if soft deletes is disabled instead.

Closes elastic#39914
polyfractal pushed a commit to polyfractal/elasticsearch that referenced this pull request Mar 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocker >bug :Distributed/Distributed A catch all label for anything in the Distributed Area. If you aren't sure, use this one. v6.7.0 v7.0.0-rc2 v7.2.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CI] AssertionError: pre-6.0 shard copy ___ unexpected to send valid local checkpoint -1
4 participants