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.

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

Rename position_offset_gap to position_increment_gap #13056

Merged
merged 1 commit into from
Aug 26, 2015
Merged

Rename position_offset_gap to position_increment_gap #13056

merged 1 commit into from
Aug 26, 2015

Conversation

xuzha
Copy link
Contributor

@xuzha xuzha commented Aug 22, 2015

Rename position_offset_gap to position_increment_gap

closes #12562

rjernst
rjernst reviewed Aug 22, 2015
View reviewed changes
core/src/test/java/org/elasticsearch/index/mapper/string/SimpleStringMappingTests.java
/**
* Test backward compatibility
*/
@Test
Copy link
Member

Choose a reason for hiding this comment

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

the @test annotation is unnecessary

nik9000
nik9000 reviewed Aug 24, 2015
View reviewed changes
core/src/main/java/org/elasticsearch/index/analysis/CustomAnalyzerProvider.java
}

int positionIncrementGap = 0;
if (Version.indexCreated(indexSettings).before(Version.V_2_0_0) ){
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need to ignore the setting in post 2.0 indexes? Why not just support both for a while? You already check above that both aren't specified.

Copy link
Member

Choose a reason for hiding this comment

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

I think we have a deprecation logger. We should probably log something when we see position_offset_gap.

Copy link

Choose a reason for hiding this comment

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

++

Copy link
Member

Choose a reason for hiding this comment

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

Why would we extend confusion?

Copy link
Member

Choose a reason for hiding this comment

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

It gives clients an upgrade window where they can safely continue to use their old mappings for a while. Its normal not to push a new version of your application in the same release window as you push Elasticsearch. In fact I'd wag my finger accusatorially at anyone that did. So you want the old code to work with the new Elasticsearch. And the only way to rig that is to support both names for a while. Either we add support for the new name in 1.7.X and re-release or we support the old name for a couple of releases.

Copy link
Member

Choose a reason for hiding this comment

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

This change maintains support for the old name on 1.x indexes. If this were a common setting, I might agree that an extended deprecation period would be helpful. But this is an advanced setting, and especially with the change to the default, users should not be setting this often. Major releases are the time to break things, and I don't think we should maintain a very confusing setting name for all of 2.x.

We can however give a nicer error message in this case. So if we detect the setting used in 2.0+, we can throw an exception early (essentially a hard deprecation message), vs letting it fall through to have the setting be unknown. Something simple like "you must now switch to position_increment_gap".

Copy link

Choose a reason for hiding this comment

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

Agreed - please add to https://github.com/elastic/elasticsearch-migration/issues as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, will update this PR later today and add it to https://github.com/elastic/elasticsearch-migration/issues.

@xuzha xuzha mentioned this pull request Aug 24, 2015
Closed
@xuzha
Copy link
Contributor Author

xuzha commented Aug 24, 2015

Updated and rebased. I think this is ready for another round of review. Thanks a lot.

nik9000
nik9000 reviewed Aug 25, 2015
View reviewed changes
core/src/test/java/org/elasticsearch/index/analysis/AnalysisModuleTests.java
getAnalysisService(settings);
fail("Analyzer has both position_offset_gap and position_increment_gap should fail");
} catch (ProvisionException e) {
assertTrue(e.getCause() instanceof IllegalArgumentException);
Copy link
Member

Choose a reason for hiding this comment

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

I'm a fan of assertThat(e.getCause(), instanceOf(IllegalArgumentException.class)); because the error reporting is better.

@nik9000
Copy link
Member

nik9000 commented Aug 25, 2015

Left a small comment otherwise LGTM.

@nik9000
Copy link
Member

nik9000 commented Aug 25, 2015

I think we will have a race between this and #12544. I think it's about to win so you might have some rebasing to do, unfortunately.

@xuzha
Copy link
Contributor Author

xuzha commented Aug 25, 2015

Thanks so much for the review, @nik9000 and @rjernst

I will do the rebase :-)

@nik9000
Copy link
Member

nik9000 commented Aug 25, 2015

I will do the rebase :-)

And now master is ready for you. I'm going to try backporting #12544 to 2.0 as requested in the pr which might complicate things more. It shouldn't be too bad.

@xuzha
Copy link
Contributor Author

xuzha commented Aug 25, 2015

Rebase, all tests passed on my machine. @nik9000 please, would you like to do a final check?

nik9000
nik9000 reviewed Aug 25, 2015
View reviewed changes
docs/reference/migration/migrate_2_0/mapping.asciidoc
@@ -384,8 +384,8 @@ The `compress` and `compress_threshold` options have been removed from the
default. If you would like to increase compression levels, use the new
<<index-codec,`index.codec: best_compression`>> setting instead.

==== position_offset_gap
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this belongs unless this is getting merged into 2.0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are going to 2.0, right? I will double check with @clintongormley

@nik9000
Copy link
Member

nik9000 commented Aug 25, 2015

Looks good. I left two minor comments that you can fix without a re-review I think. Other than that it looks good to me.

@nik9000 nik9000 added >breaking :Search Foundations/Mapping Index mappings, including merging and defining field types v2.0.0 labels Aug 25, 2015
The name "position_offset_gap" is confusing because Lucene has three
fb2be6d 
similar sounding things:

* Analyzer#getPositionIncrementGap
* Analyzer#getOffsetGap
* IndexOptions.DOCS_AND_FREQS_AND_POSITIONS_AND_OFFSETS and
* FieldType#storeTermVectorOffsets

Rename position_offset_gap to position_increment_gap
closes elastic#13056
@xuzha xuzha merged commit fb2be6d into elastic:master Aug 26, 2015
@xuzha xuzha deleted the xu-rename-offset branch August 27, 2015 05:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
>breaking :Search Foundations/Mapping Index mappings, including merging and defining field types v2.0.0-beta2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Rename position_offset_gap to position_increment_gap
4 participants
@xuzha @nik9000 @clintongormley @rjernst