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

Deprecate validate_* and normalize_* #10248

Closed
wants to merge 3 commits into from

Conversation

nknize
Copy link
Contributor

@nknize nknize commented Mar 25, 2015

No need to carry 3 validate and 3 normalize options in GeoPointFieldMapping. This is just causing confusion. This change deprecates the validate_lat, validate_lon, normalize_lat, and normalize_lon options on the geo_point field mapping and simplifies the options by using just validate and normalize, respectively. Geo filters have been updated to include a validate and normalize option to provide consistency with the field mapping defaults. Unit tests and documentation have also been updated.

closes #10170

@nknize nknize added >breaking v2.0.0-beta1 :Analytics/Geo Indexing, search aggregations of geo points and shapes v1.6.0 >deprecation labels Mar 25, 2015
iterator.remove();
} else if (fieldName.equals("validate_lon")) {
builder.validateLon = XContentMapValues.nodeBooleanValue(fieldNode);
iterator.remove();
if (parserContext.indexVersionCreated().onOrAfter(Version.V_1_6_0)) {
Copy link
Member

Choose a reason for hiding this comment

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

Given how we've done other deprecations and removals, I think we need to maintain backcompat for at least reading and ignoring these settings in 2.0? Also, I think it is better to add move the version check to the else if and invert, then have a comment like // ignore legacy setting inside the block. For newer indexes, the setting would fall through and show up as an unknown setting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like that idea. I'm a big fan of avoiding exceptions if at all possible but wasn't sure how we've handled bwc in other cases. That will let us remove the 'breaking' label too? Committing a change shortly for review. Will also need to add more thorough testing before merging. I just needed this valuable feedback.

@rjernst
Copy link
Member

rjernst commented Mar 25, 2015

@nknize I left a couple comments.

@clintongormley Maybe you have some thoughts on whether the breaking change should happen for 1.6 or 2.0?

No need to carry 3 validate and 3 normalize options in GeoPointFieldMapping. This is just causing confusion so this change deprecates the validate_lat, validate_lon, normalize_lat, and normalize_lon options and simply uses validate and normalize, respectively. Geo filters have also been updated to include a validate and normalize option to provide consistency with the mapping defaults. Unit tests and documentation have been updated.

closes elastic#10170
@dakrone
Copy link
Member

dakrone commented May 4, 2015

@clintongormley I think deprecate in 1.6.0 and remove in 2.0.0, what do you think?

@clintongormley
Copy link

Wondering if we should make the whole change (including removing validate) in 2.0. So just mark the options as deprecated in 1.6, eg:

|`validate` | deprecated[1.6.0, This parameter will be removed in 2.0] Set to `false` to accept geo points with invalid latitude or
longitude (default is `true`). *Note*: Validation only works when
normalization has been disabled. This option will be deprecated and removed

@clintongormley
Copy link

@nknize any movement here?

@clintongormley clintongormley changed the title [GEO] Deprecate validate_* and normalize_* Deprecate validate_* and normalize_* Jun 6, 2015
@nknize
Copy link
Contributor Author

nknize commented Jun 9, 2015

@clintongormley looks like my issue notification was eaten by build failures. I'll deprecate in 1.6.1 and completely remove in 2.0. This one has hung around long enough.

@clintongormley
Copy link

@nknize perhaps do something similar to the suggestion here: #11161 (comment)

@martijnvg martijnvg added v1.7.1 and removed v1.6.1 labels Jul 16, 2015
@martijnvg
Copy link
Member

Bumping the version up to 1.7.1 for the today's release.

@nknize
Copy link
Contributor Author

nknize commented Jul 16, 2015

closing this PR in favor of #12300

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/Geo Indexing, search aggregations of geo points and shapes >deprecation v1.7.1 v2.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[GEO] Deprecate 'validate_*' options in GeoPointMapping
6 participants