-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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 soft deletes #1903
Enforce soft deletes #1903
Conversation
This commit enforces soft deletes as required in 2.0.0. Signed-off-by: Nicholas Walter Knize <[email protected]>
Can one of the admins verify this patch? |
Signed-off-by: Nicholas Walter Knize <[email protected]>
+ request.index() | ||
+ "]." | ||
if (IndexSettings.INDEX_SOFT_DELETES_SETTING.get(indexSettings) == false | ||
&& IndexMetadata.SETTING_INDEX_VERSION_CREATED.get(indexSettings).onOrAfter(Version.V_2_0_0)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit confused...this is server code that will run inside a version 2.0.0+ instance of the service. Why is this check necessary? Why can't the check just be
if (!IndexSettings.INDEX_SOFT_DELETES_SETTING.get(indexSettings)) {
throw ...
}
Taking it a bit further, why can't we just remove the index.soft_deletes.enabled
setting all together so that it cannot be specified since the soft delete behavior is not configurable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andrross afaik when migrating from let say 1.x to 2.x, you could still use "old" index from 1.x in 2.x: in this case the version of the index will be 1.x
, with all the settings implied. If we drop this check, the "old" indices would behave differently (or fail to load if we throw an exception).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. So we'll continue to support the "soft delete disabled behavior" after this change because it is possible to migrate an index from an older version with index.soft_deletes.enabled=false to a 2.0+ version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, this is my understanding
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct. We need to be able to support the parameter through 2.x for bwc purposes.
This PR enforces soft deletes as required in 2.0.0.
relates #1674