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

Deprecation info for joda-java migration #41956

Merged
merged 54 commits into from May 29, 2019

Conversation

Projects
None yet
6 participants
@pgomulka
Copy link
Contributor

commented May 8, 2019

Implement deprecation checks for joda incompatible patterns

relates #42010

pgomulka added some commits May 7, 2019

@pgomulka pgomulka self-assigned this May 8, 2019

@elasticmachine

This comment has been minimized.

Copy link

commented May 8, 2019

@rjernst
Copy link
Member

left a comment

A few comments

Show resolved Hide resolved ...ework/src/main/java/org/elasticsearch/bootstrap/BootstrapForTesting.java Outdated

static Map<String, String> JODA_PATTERNS_DEPRECATIONS = new LinkedHashMap<>();
static {
JODA_PATTERNS_DEPRECATIONS.put("Y", "'Y' year-of-era becomes 'y'. 'Y' means week-based-year.");

This comment has been minimized.

Copy link
@rjernst

rjernst May 8, 2019

Member

We already have these deprecation messages in Joda, which are logged when the patterns are parsed. This seems to be doubling them? Additionally, I think some of these are too broad, like the check for y, which users wouldn't need to do anything about. Any deprecation messages should be actionable.

This comment has been minimized.

Copy link
@pgomulka

pgomulka May 9, 2019

Author Contributor

Yes - that was just a draft, but next step is to have those messages in one place and use in both Joda & Deprecation API.
for y pattern check I think it would be fair to notify users about this as it changes to u (I missed this on previous commit). I suppose some users would just ignore this check specifically as a difference is very subtle, but we should at least let them know.

Some of the patterns are not actionable though - like C century is no longer present in java.time. Again users should be aware about this, as their pattern will probably no longer work. Action would be to change your business logic?

Show resolved Hide resolved ...ain/java/org/elasticsearch/xpack/deprecation/IndexDeprecationChecks.java Outdated

@pgomulka pgomulka added the WIP label May 8, 2019

@gwbrown

This comment has been minimized.

Copy link
Contributor

commented May 8, 2019

This approach seems reasonable to me, the warning it generates is a little verbose but otherwise should be fine (and it's not like we don't already have some very verbose warnings).

As others have noted, there's a fair bit of commented out code, etc. left, but I assume that's why this PR is still marked as a draft/WIP.

pgomulka added some commits May 9, 2019

@pgomulka

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2019

there are a lot of test failing as the deprecation now includes y and Z (and that adds deprecation to headers). I understand @rjernst and @gwbrown concerns that this might be a bit too verbose.
At the same time, since y has a slightly different meaning now in java.time and Z behaves a little bit different I think it would make sens to warn users about this.
@spinscale what is your view on this?

@pgomulka pgomulka force-pushed the pgomulka:feature/deprecation_info_joda branch from 9152886 to c94bbe7 May 10, 2019

pgomulka added some commits May 11, 2019

@@ -41,7 +41,9 @@ PUT _template/template_1
}
--------------------------------------------------
// CONSOLE
// TESTSETUP
// TEST[setup:name]

This comment has been minimized.

Copy link
@pgomulka

pgomulka May 21, 2019

Author Contributor

I need to figure out how to fix Docs tests. At the moment they fail because of warnings

This comment has been minimized.

Copy link
@rjernst

rjernst May 21, 2019

Member

Normally we would update the docs to not use deprecated functionality. Is that possible here?

This comment has been minimized.

Copy link
@pgomulka

pgomulka May 22, 2019

Author Contributor

it would - actually it would be much easier if I just use a new pattern with 8 prefix. But should this be done across all the docs?
That change is to fix the DocsClientYamlTestSuiteIT. The options is to prefix with8, assert about warnings, disable warning checks alltogether?

@gwbrown
Copy link
Contributor

left a comment

Left a few comments, mostly minor.

You mentioned wanting to have more Javadoc and tests, which I agree with as well, plus the commented-out code and TODOs left which I didn't address as this is still marked WIP. Otherwise I still don't see any major problems, and it's already much improved from the last time I looked at this. Thanks for doing this!

import static org.hamcrest.Matchers.is;

public class JavaJodaTimeDuellingTests extends ESTestCase {
@Override
protected boolean enableWarningsCheck() {
// disable warning checks as deprecated patterns are used to compare Joda vs Java results (y - year and Z zone offset)

This comment has been minimized.

Copy link
@gwbrown

gwbrown May 21, 2019

Contributor

I would update this note to say that these warnings as tested in JodaWarningTests already, so that's why we don't test them here.

Pretend I copy/pasted this onto every iteration of this comment, but I'm not going to because there's a lot of them.

@@ -28,6 +28,11 @@
import java.util.Arrays;

public class RootObjectMapperTests extends ESSingleNodeTestCase {
@Override
protected boolean enableWarningsCheck() {
// disable warning checks as deprecated patterns are used to compare Joda vs Java results (y - year and Z zone offset)

This comment has been minimized.

Copy link
@gwbrown

gwbrown May 21, 2019

Contributor

I get doing this for date-specific tests, but there's a lot of classes that have the warnings check disabled here, including some very broad ones. We should be careful about doing this so widely, although I could see an argument that this is okay if it would be a lot of work to use assertWarnings everywhere given these will only be going into 6.8, which should pretty much only be getting bugfixes at this point.

This comment has been minimized.

Copy link
@pgomulka

pgomulka May 22, 2019

Author Contributor

where possible I used explicitly assertWarnings about these patterns, but often it was just not feasible and would require adding lot of them.
and agree, that is only going to be in 6.8 so hopefully would not reduce confidence in these tests

Show resolved Hide resolved ...ain/java/org/elasticsearch/xpack/deprecation/IndexDeprecationChecks.java
Show resolved Hide resolved ...ain/java/org/elasticsearch/xpack/deprecation/IndexDeprecationChecks.java Outdated
private static Map<String, String> JODA_PATTERNS_DEPRECATIONS = new LinkedHashMap<>();

static {
JODA_PATTERNS_DEPRECATIONS.put("Y", "'Y' year-of-era becomes 'y'. Use 'Y' for week-based-year.");

This comment has been minimized.

Copy link
@gwbrown

gwbrown May 21, 2019

Contributor

I think we should make this a bit clearer as to what we're recommending be done. Something like this:

Suggested change
JODA_PATTERNS_DEPRECATIONS.put("Y", "'Y' year-of-era becomes 'y'. Use 'Y' for week-based-year.");
JODA_PATTERNS_DEPRECATIONS.put("Y", "'Y' year-of-era should be replaced with 'y'. Use 'Y' for week-based-year.");

And similar for the rest of these.

}
}
String combinedWarning = warnings.stream()
.collect(Collectors.joining(";"));

This comment has been minimized.

Copy link
@droberts195

droberts195 May 24, 2019

Contributor

Since this is building a message for the end user it would be nice to put a space after the semi-colon.

This comment has been minimized.

Copy link
@pgomulka

pgomulka May 24, 2019

Author Contributor

good point, this would be more readable with a space

"https://www.elastic.co/guide/en/elasticsearch/reference/7.0/breaking-changes-7.0.html#breaking_70_java_time_changes",
"This index has date fields with deprecated formats: ["+
"[type: _doc, field: date_time_field_Y, format: dd-CC||MM-YYYY, " +
"suggestion: 'C' century of era is no longer supported.;"+

This comment has been minimized.

Copy link
@droberts195

droberts195 May 24, 2019

Contributor

The line break disguises the fact that there's no space after the semi-colon here. At the moment the message is ...supported.;'Y' year-of-era.... If you accept my other suggestion you'll need to add a space here.

This comment has been minimized.

Copy link
@pgomulka

pgomulka May 24, 2019

Author Contributor

You are right, will add a space after a semi-colon. Should be more readable

pgomulka added some commits May 24, 2019

@pgomulka

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2019

This PR has grown significantly which is sad as I only expected this to be a quick fix.
There are a lot of tests that I had to change, where possible I explicitly asserted about new warnings in a headers, but sometimes I had to turn that check off as it would make this PR just massive.

I have added some more tests to cover the message formatting and different scenarios of the pattern.

@gwbrown @rjernst @droberts195 would you be able to re-review this again?

@gwbrown
Copy link
Contributor

left a comment

The deprecation check LGTM, though I left a couple comments. Others may want to weigh in on the rest.

pgomulka and others added some commits May 25, 2019

Update x-pack/plugin/deprecation/src/main/java/org/elasticsearch/xpac…
…k/deprecation/IndexDeprecationChecks.java

Co-Authored-By: Gordon Brown <gordon.brown@elastic.co>
@pgomulka

This comment has been minimized.

Copy link
Contributor Author

commented May 27, 2019

added support for deprecation info on pipelines with date and date_index_name processors
Also I disabled only joda deprecation checks by default only. This was spreading far too much and was not really giving much more confidence as assertions we usually about the same pattern.

pgomulka added some commits May 27, 2019

@gwbrown
Copy link
Contributor

left a comment

Left a couple minor comments but otherwise LGTM. No need for another round of review after addressing these unless you make other substantial changes.


//in joda 'Z' was able to parse 'Z' zulu but in java it fails. You have to use 'X' to do that.
//Caused by: java.time.format.DateTimeParseException: Text '2019-01-01T01:01:01.001Z' could not be parsed at index 23
//assertSameMillis("2019-01-01T01:01:01.001Z","YYYY-MM-dd'T'HH:mm:ss.SSSZ","8yyyy-MM-dd'T'HH:mm:ss.SSSZ");

This comment has been minimized.

Copy link
@gwbrown

gwbrown May 28, 2019

Contributor

I think these lines might be left over from something? Otherwise I'm not sure why they're here.

This comment has been minimized.

Copy link
@pgomulka

pgomulka May 29, 2019

Author Contributor

This whole test case testIncompatiblePatterns was meant to use examples of patterns that are incompatible between joda-java and show how to fix them.
For 'Z' we should go for 'X' so I will just remove this comment.

Show resolved Hide resolved ...n/java/org/elasticsearch/xpack/deprecation/ClusterDeprecationChecks.java Outdated

pgomulka and others added some commits May 29, 2019

Update x-pack/plugin/deprecation/src/main/java/org/elasticsearch/xpac…
…k/deprecation/ClusterDeprecationChecks.java

Co-Authored-By: Gordon Brown <gordon.brown@elastic.co>

@pgomulka pgomulka merged commit 728c5d1 into elastic:6.8 May 29, 2019

8 checks passed

CLA All commits in pull request signed
Details
elasticsearch-ci/1 Build finished.
Details
elasticsearch-ci/2 Build finished.
Details
elasticsearch-ci/bwc Build finished.
Details
elasticsearch-ci/default-distro Build finished.
Details
elasticsearch-ci/docbldesx Build finished.
Details
elasticsearch-ci/oss-distro-docs Build finished.
Details
elasticsearch-ci/packaging-sample Build finished.
Details
@mark-vieira

This comment has been minimized.

Copy link
Contributor

commented May 29, 2019

FYI, this change is breaking some tests in BWC compatible branches. We'll need to update org.elasticsearch.backwards.MixedClusterClientYamlTestSuiteIT in 7.1, 7.2 and 7.x.

https://scans.gradle.com/s/z2xy2pasutgty/tests/z3ejz3hl3i73s-wptfavgv7zdvi

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.