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

Jump to bottom

Allow for backwards compatibility for unix timestamp in pre 2.x indices #11515

Merged
merged 1 commit into from Jun 25, 2015
Merged

Allow for backwards compatibility for unix timestamp in pre 2.x indices #11515

merged 1 commit into from Jun 25, 2015

Conversation

spinscale
Copy link
Contributor

In order to be backwards compatible, indices created before 2.x must support
indexing of a unix timestamp and its configured date format. Indices created
with 2.x must configure the epoch_millis date formatter in order to
support this. Also the numeric_resolution parameter is not needed anymore,
as this is configured by either using epoch_millis or epoch_seconds, however
it must still be supported for older indices.

Relates #10971

@clintongormley clintongormley changed the title Dates: Allow for backwards compatibility for unix timestamp in pre 2.x indices Allow for backwards compatibility for unix timestamp in pre 2.x indices Jun 8, 2015
rjernst
rjernst reviewed Jun 8, 2015
View reviewed changes
src/main/java/org/elasticsearch/action/index/IndexRequest.java
@@ -557,13 +559,22 @@ public boolean autoGeneratedId() {
return this.autoGeneratedId;
}

private static Version getIndexVersionFromIndex(String index, MetaData metaData) {
Copy link
Member

Choose a reason for hiding this comment

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

Why not use Version.indexeCreated? How is it possible to be looking up created version for an index that doesn't exist?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@rjernst
Copy link
Member

rjernst commented Jun 8, 2015

@spinscale Thanks for adding this bwc. I left a couple comments.

@spinscale
Copy link
Contributor Author

After a discussion with Boaz, we should add a configurable mapping option, that supports parsing a unix timestamp by default, but can be disabled via the mapping in order to not break existing applications

@jpountz
Copy link
Contributor

jpountz commented Jun 9, 2015

@spinscale @bleskes Can you elaborate on why we need an option? We've been making lots of efforts to reduce options on mappings so adding a new one feels a bit like going backwards?

@bleskes
Copy link
Contributor

bleskes commented Jun 9, 2015

@jpountz it is my understanding the idea is not to introduced another param but rather change the default date parsing such that it does what the old code used to do (give preference to unix time stamps) but this time all the parsing goes through the proper channels (field mappers), making it simpler and allowing people which don't like it the ability to change it.

@jpountz
Copy link
Contributor

jpountz commented Jun 9, 2015

If we only change the default behaviour based on when the index was created then I'm good.

@bleskes
Copy link
Contributor

bleskes commented Jun 9, 2015

@jpountz we do it all the time as we used to di this hard coded in code. Now it will be configurable (and the same by default)

@s1monw s1monw added the blocker label Jun 22, 2015
@s1monw
Copy link
Contributor

s1monw commented Jun 22, 2015

@spinscale any idea when you can work on this / get this in?

@spinscale spinscale force-pushed the 1506-unix-timestamp-bwc-compat branch from ec77f90 to 8898f41 Compare June 23, 2015 12:56
@spinscale
Copy link
Contributor Author

rebased against master and fixed some tests, need to add more tests to ensure expected behaviour

@spinscale spinscale force-pushed the 1506-unix-timestamp-bwc-compat branch from 8898f41 to 11ceb48 Compare June 24, 2015 12:07
@spinscale
Copy link
Contributor Author

ok added tests, that do some of the scenarios explained in #10971 (will leave the dynamic date detection in its own PR)

would be happy go get another review here, and an agreement if we should leave the version specific date formatter in the mapper or move it out. thx! /cc @rjernst

rjernst
rjernst reviewed Jun 24, 2015
View reviewed changes
core/src/test/java/org/elasticsearch/index/mapper/date/DateBackwardsCompatibilityTests.java
*/
public class DateBackwardsCompatibilityTests extends ElasticsearchSingleNodeTest {

long dateInMillis = 1435073872l * 1000; // Tue Jun 23 17:37:52 CEST 2015
Copy link
Member

Choose a reason for hiding this comment

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

why isn't this a local var in the couple tests that use it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

rjernst
rjernst reviewed Jun 24, 2015
View reviewed changes
core/src/test/java/org/elasticsearch/index/mapper/date/SimpleDateMappingTests.java
@@ -289,7 +288,7 @@ public void testIgnoreMalformedOption() throws Exception {
.endObject()
.bytes());
} catch (MapperParsingException e) {
assertThat(e.getCause(), instanceOf(MapperParsingException.class));
assertThat(e.getCause(), instanceOf(IllegalArgumentException.class));
Copy link
Member

Choose a reason for hiding this comment

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

can we check part of the message so we know we got the right exception? ditto for the other cases like this in this file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed for all the cases, checking content of e.getMessage() now

@rjernst
Copy link
Member

rjernst commented Jun 24, 2015

@spinscale I left some more comments.

@spinscale spinscale force-pushed the 1506-unix-timestamp-bwc-compat branch from 11ceb48 to 7d53a06 Compare June 25, 2015 11:05
@spinscale
Copy link
Contributor Author

worked on all your review comments, except the documentation one, which I think should be kept there to not confuse users trying to look numeric_resolution up

@spinscale spinscale force-pushed the 1506-unix-timestamp-bwc-compat branch from 7d53a06 to 81a5beb Compare June 25, 2015 13:43
@rjernst
Copy link
Member

rjernst commented Jun 25, 2015

LGTM, thanks! I left one small comment on the wording for migration docs.

@spinscale
Dates: Be backwards compatible with pre 2.x indices
23cf9af 
In order to be backwards compatible, indices created before 2.x must support
indexing of a unix timestamp and its configured date format. Indices created
with 2.x must configure the `epoch_millis` date formatter in order to
support this.

Relates elastic#10971
@spinscale spinscale force-pushed the 1506-unix-timestamp-bwc-compat branch from 81a5beb to 23cf9af Compare June 25, 2015 15:21
@spinscale spinscale merged commit 23cf9af into elastic:master Jun 25, 2015
@kevinkluge kevinkluge removed the review label Jun 25, 2015
@clintongormley clintongormley added :Search/Mapping Index mappings, including merging and defining field types and removed :Dates labels Feb 13, 2018
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
blocker >enhancement :Search/Mapping Index mappings, including merging and defining field types v2.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@spinscale @rjernst @jpountz @bleskes @s1monw @clintongormley @kevinkluge