Skip to content

CCR: mention how to enable soft deletes in error message when not enabled #36476

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Dec 12, 2018
Merged

CCR: mention how to enable soft deletes in error message when not enabled #36476

merged 2 commits into from
Dec 12, 2018

Conversation

@bleskes
Copy link
Contributor

@bleskes bleskes commented Dec 11, 2018

No description provided.

@bleskes bleskes added >non-issue v7.0.0 :Distributed Indexing/CCR Issues around the Cross Cluster State Replication features v6.6.0 labels Dec 11, 2018
@bleskes bleskes requested a review from martijnvg December 11, 2018 12:03
@elasticmachine
Copy link
Collaborator

elasticmachine commented Dec 11, 2018

Pinging @elastic/es-distributed

martijnvg
martijnvg approved these changes Dec 11, 2018
View reviewed changes
Copy link
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

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

LGTM - Do think tests need to be adjusted because of this change. (LocalFollowIndexIT#testDoNotCreateFollowerIfLeaderDoesNotHaveSoftDeletes(...) and TransportResumeFollowActionTests#testValidation(...))

@bleskes
Copy link
Contributor Author

bleskes commented Dec 11, 2018

Thanks @martijnvg, I was optimistically relying on CI to be bubble up tests that rely on this string (if at all). Thanks for the heads up.

jasontedor
jasontedor requested changes Dec 11, 2018
View reviewed changes
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.

🙏

...ck/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/TransportPutFollowAction.java Outdated
Copy link
Member

@jasontedor jasontedor Dec 11, 2018

Choose a reason for hiding this comment

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

Please use IndexSettings.INDEX_SOFT_DELETES_SETTING.getKey() so we have fewer places we have to search the code base for the hard string for, if ever we need to.

Copy link
Contributor Author

@bleskes bleskes Dec 11, 2018

Choose a reason for hiding this comment

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

doh. Should have done this from the get go.

@bleskes bleskes changed the title CCR: mention how to enable soft deletes in error message when non enabled CCR: mention how to enable soft deletes in error message when not enabled Dec 11, 2018
@bleskes bleskes force-pushed the ccr_soft_delete_not_enabled_error branch from c520ed1 to a3af273 Compare December 12, 2018 09:56
@bleskes bleskes removed the v7.0.0 label Dec 12, 2018
@bleskes bleskes changed the base branch from master to 6.x December 12, 2018 09:57
@bleskes
Copy link
Contributor Author

bleskes commented Dec 12, 2018

check this please

@bleskes
Copy link
Contributor Author

bleskes commented Dec 12, 2018 •
edited
Loading

@martijnvg @jasontedor FYI - I rebased this against 6.x . With the merge of #36141 which enables soft deletes as a default, I don't think this makes sense for master any more.

@bleskes bleskes merged commit f8e4b30 into elastic:6.x Dec 12, 2018
@bleskes bleskes deleted the ccr_soft_delete_not_enabled_error branch December 12, 2018 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@martijnvg martijnvg martijnvg approved these changes

@jasontedor jasontedor jasontedor approved these changes

Assignees

No one assigned

Labels

:Distributed Indexing/CCR Issues around the Cross Cluster State Replication features >non-issue v6.6.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@bleskes @elasticmachine @martijnvg @jasontedor