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

Enable GeoPointV2 with backward compatibility testing #14667

Closed
wants to merge 0 commits into from

Conversation

Projects
None yet
3 participants
@nknize
Copy link
Member

commented Nov 10, 2015

This PR removes all noreleases and cuts over to Lucene 5.4 GeoPointField type. Included are randomized testing updates to unit and integration test suites for ensuring full backward compatability with existing geo_point indexes.

closes #9859
closes #10761
closes #11159

@nknize

This comment has been minimized.

Copy link
Member Author

commented Nov 11, 2015

@rjernst if you wouldn't mind giving this a once over. The majority of the change is removing the noreleases and cutting over to .before(V_2_2_0). But I've also added random index version creation to test backward compatibility.

@rjernst

View changes

core/src/test/java/org/elasticsearch/index/fielddata/AbstractFieldDataTestCase.java Outdated
@@ -94,8 +95,8 @@ protected boolean hasDocValues() {
} else if (type.getType().equals("byte")) {
fieldType = MapperBuilders.byteField(fieldName).docValues(docValues).fieldDataSettings(type.getSettings()).build(context).fieldType();
} else if (type.getType().equals("geo_point")) {
// norelease update to .before(Version.V_2_2_0 once GeoPointFieldV2 is fully merged
if (indexService.getIndexSettings().getIndexVersionCreated().onOrBefore(Version.CURRENT)) {
if (type.getSettings().getAsVersion(IndexMetaData.SETTING_VERSION_CREATED, Version.CURRENT).before(Version.V_2_2_0)

This comment has been minimized.

Copy link
@rjernst

rjernst Nov 12, 2015

Member

Why both checks? The version created should be the same (they point to the same setting), right?

This comment has been minimized.

Copy link
@nknize

nknize Nov 12, 2015

Author Member

They don't. But I agree they probably should. At the moment AbstractFieldDataTestCase.setup() creates the index settings which doesn't have any version randomization. When I added randomization it threw all sorts of backcompat exceptions in other tests unrelated to this GeoPointField feature. To keep that change separate I went ahead and added the version randomization I need for GeoField backcompat testing in AbstractGeoFieldDataTestCase.getFieldDataSettings which has the side effect of seting the fieldType settings separately from the index settings.

This comment has been minimized.

Copy link
@nknize

nknize Nov 12, 2015

Author Member

I think revisiting the whole fielddata test package would be a worthwhile task for test triage?

This comment has been minimized.

Copy link
@rjernst

rjernst Nov 12, 2015

Member

Well, I think this needs to be fixed here. There is no index created version in field data settings, this is an artificial thing that it sounds like you have added to workaround some other issue.

This comment has been minimized.

Copy link
@nknize

nknize Nov 12, 2015

Author Member

Indeed! That other issue was setting version randomization to cover pre-2.0, when docvalues were not enabled. I pushed an update that purges this settings inconsistency.

@rjernst

View changes

plugins/lang-groovy/src/test/java/org/elasticsearch/messy/tests/GeoDistanceTests.java Outdated
@@ -222,12 +232,15 @@ public void testSimpleDistance() throws Exception {
}

public void testDistanceSortingMVFields() throws Exception {
Version version = VersionUtils.randomVersionBetween(random(), Version.V_1_0_0, Version.CURRENT);

This comment has been minimized.

Copy link
@rjernst

rjernst Nov 12, 2015

Member

I'm a little concerned about all of these "choose a random version" because it will severely reduce the test coverage for the new code? If we are concerned about backcompat, lets have dedicated backcompat tests for pre 2.2 behavior being maintained...

This comment has been minimized.

Copy link
@nknize

nknize Nov 12, 2015

Author Member

I like that idea. I renamed issue #14562. That issue will cover removing all of these random versions and adding dedicated backcompat testing. For now I'll change the random version range as follows:

  • for master: Version.V_2_0_0 to Version.CURRENT
  • for 2.x: Version.V_1_0_0 to Version.CURRENT

This way we cover backcompat testing with supported versions and increase coverage for the new code in master.

This comment has been minimized.

Copy link
@rjernst

rjernst Nov 12, 2015

Member

Ok, I'm fine with that..

@rjernst

This comment has been minimized.

Copy link
Member

commented Nov 12, 2015

The cutover looks fine, but I am concerned about the tests.

@rjernst

This comment has been minimized.

Copy link
Member

commented Nov 12, 2015

LGTM, thanks for the fixes!

@nknize nknize removed the review label Nov 12, 2015

@nknize nknize force-pushed the nknize:geopointv2_enable_3.0 branch 4 times, most recently Nov 12, 2015

@nknize nknize closed this Nov 13, 2015

@nknize nknize force-pushed the nknize:geopointv2_enable_3.0 branch to dc77815 Nov 13, 2015

@nknize nknize deleted the nknize:geopointv2_enable_3.0 branch May 27, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.