Support joda style date patterns in 7.x #52555
Conversation
|
Pinging @elastic/es-core-infra (:Core/Infra/Core) |
|
@pgomulka there seems to be some unrelated changes here? Are you completely up-to-date with |
pgomulka
force-pushed
the
joda/enable_joda_indices
branch
from
9990233 to
9b79406
Compare
rolling upgrade test for joda-java support for java.style created in 6 tests passing revert not needed changes
pgomulka
force-pushed
the
joda/enable_joda_indices
branch
from
75b3458 to
04d376a
Compare
|
@pugnascotia yes, this is probably because of file encoding and line separators on windows.. trying to fix.. |
|
@elasticmachine test this please |
| if (hasPatternChanged || Objects.equals(builder.locale, formatter.locale()) == false) { | ||
| fieldType().setDateTimeFormatter(DateFormatter.forPattern(pattern).withLocale(locale)); | ||
| DateFormatter dateTimeFormatter; | ||
| // if (Joda.isJodaStyleIndex(context,pattern) ) { |
There was a problem hiding this comment.
yes - I am trying to find a good example where the test for range fields fails in 7 with joda pattern. Luckily the one with 'YYYY' passes -although the values are incorrectly parsed
|
@elasticmachine run elasticsearch-ci/bwc |
|
@elasticmachine update branch |
|
it was failing the build because I forgot to use refresh when indexing docs. |
| response = performRequest(iteratorNodeTuple, internalRequest, null); | ||
| } | ||
| return response; | ||
| } |
There was a problem hiding this comment.
We shouldn't add this method to the REST client, it's trappy by design given how it ignores all responses but the last one?
There was a problem hiding this comment.
good point, this should not be available outside tests
| * Hence a skip assume section in init() | ||
| * | ||
| * @see org.elasticsearch.search.DocValueFormat.DateTime | ||
| */ |
There was a problem hiding this comment.
Thanks for adding javadocs to this test suite. This isn't common practice in our code base, but is definitely useful in this case.
| * | ||
| * @see org.elasticsearch.search.DocValueFormat.DateTime | ||
| */ | ||
| public class DateFieldsIT extends AbstractRollingTestCase { |
There was a problem hiding this comment.
Can you use a more specific name, something like JodaCompatibilityIT?
| return options.build(); | ||
| } | ||
|
|
||
| private void assertSearchResponse(Response searchResp) throws IOException { |
There was a problem hiding this comment.
maybe use a different name, when I first saw the method I assumed it would check for shard failures, like ElasticsearchAssertions#assertSearchResponse, but this one actually checks the content.
| search.setOptions(ignoreWarnings()); | ||
|
|
||
| Response searchResp = client().performRequest(search,3); | ||
| assertEquals(HttpStatus.SC_OK, searchResp.getStatusLine().getStatusCode()); |
There was a problem hiding this comment.
Can we go further and assert that the total number of shards is equal to the number of successful shards?
There was a problem hiding this comment.
good idea. will do
| * all shards are allocated on the same node and client is connecting to it. | ||
| * Because of this warnings assertions have to be ignored. | ||
| * | ||
| * A special flag on DocValues is used to indicate that an index was created in 6.x and has a joda pattern. |
There was a problem hiding this comment.
the flag is used during serialization of DocValueFormat.DateTime
I have reworded the javadoc
|
|
||
| public static boolean isJodaPattern(Version version, String pattern) { | ||
| return version.before(Version.V_7_0_0) | ||
| && version.after(Version.V_6_0_0) |
There was a problem hiding this comment.
I think you meant onOrAfter. But this makes me wonder whether we need this condition of the boolean expression, pre-6.x indices are not supported anyway in 7.
There was a problem hiding this comment.
agree - will remove && version.after(Version.V_6_0_0)
I meant to make it explicit, but I think the context in v7 is clear that pre6x is not supported
| } | ||
| }; | ||
|
|
||
| public static boolean isJodaPattern(Version version, String pattern) { |
| isJoda = in.readBoolean(); | ||
| } else { | ||
| //when received a stream from 6.0-6.latest Node it can be java if starts with 8 otherwise joda | ||
| // If a stream is from [7.0 - 7.7) it will assume (sometimes incorrectly) that the date is java |
There was a problem hiding this comment.
can you add more details about when it gets incorrect? If my understanding is correct, this happens if an index is created on 6.x and the cluster is later upgraded to 7.x (x<7)?
There was a problem hiding this comment.
you are correct. added more details about this. Let me know if it is clear now
|
@elasticmachine run elasticsearch-ci/2 |
|
@elasticmachine update branch |
|
@elasticmachine run elasticsearch-ci/packaging-sample-matrix-windows |
| ); | ||
| Response resp = client().performRequest(putDoc); | ||
| assertEquals(HttpStatus.SC_CREATED, resp.getStatusLine().getStatusCode()); | ||
| // flush(endpoint,true); |
when upgrading from 7.7+ ES will send out a flag indicating if a pattern is of joda style. This is only used to support joda style indices in 7.x, in 8 we no longer support this. All indices in 8 should use java style pattern. Hence we can ignore this flag. Similarly when writing from v8 to v7.7+ we should always send false flag. relates #52555 relates #53478 closes #53477
If an index was created in version 6 and contain a date field with a joda-style pattern it should still be allowed to search and insert document into it.
Those created in 6 but date pattern starts with 8, it should be considered as java style.