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

[Java.time] Retain prefixed date pattern in formatter #48703

Merged
merged 12 commits into from
Nov 27, 2019
Merged

[Java.time] Retain prefixed date pattern in formatter #48703

merged 12 commits into from
Nov 27, 2019

Conversation

pgomulka
Copy link
Contributor

@pgomulka pgomulka commented Oct 30, 2019 •
edited
Loading

JavaDateFormatter should keep the pattern with the prefixed 8 as it will be used for serialisation. The stripped pattern should be used for the enclosed formatters.

closes #48698

@pgomulka pgomulka added the WIP label Oct 30, 2019
@pgomulka pgomulka changed the title [Java.time] Retain prefixed date patterin in formatter WIP [Java.time] Retain prefixed date patterin in formatter Oct 30, 2019
@pgomulka
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/bwc

@pgomulka pgomulka added the :Core/Infra/Core Core issues without another label label Nov 4, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (:Core/Infra/Core)

@pgomulka pgomulka changed the title WIP [Java.time] Retain prefixed date patterin in formatter [Java.time] Retain prefixed date patterin in formatter Nov 4, 2019
@pgomulka
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/2

@pgomulka pgomulka requested a review from rjernst November 13, 2019 16:30
@pgomulka
Copy link
Contributor Author

ok to test

@pgomulka
Copy link
Contributor Author

@elasticmachine update branch

@jaymode jaymode added v6.8.6 and removed v6.8.5 labels Nov 19, 2019
@pgomulka pgomulka changed the title [Java.time] Retain prefixed date patterin in formatter [Java.time] Retain prefixed date pattern in formatter Nov 20, 2019
@pgomulka
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/1

@pgomulka pgomulka mentioned this pull request Nov 20, 2019
Closed
@pgomulka
Copy link
Contributor Author

@rjernst the master-fwc is failing, but this is most likely because of #49378

rjernst
rjernst approved these changes Nov 25, 2019
View reviewed changes
Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

The change looks good. Can you clarify which versions this is intended for? I see the v8.0.0 label, but the PR is against 7.x (which I believe is correct).

@pgomulka
Copy link
Contributor Author

@rjernst this I think should go to v8, v7.x and v6.8
I had this problem reproduced when running a bwc test on 7.x (this pr). Master node on 6.8 and non-master node on 7.x was stripping the 8.

I wasn't able to confirm this failing same way when Master node was on 7 and non-master node was on 6.8. It was always passing for me. I still think this should go to 6.8 branch as well.

I am yet to try to reproduce this on v8 and v7 mixed cluster.

@pgomulka pgomulka merged commit 502873b into elastic:7.x Nov 27, 2019
pgomulka added a commit to pgomulka/elasticsearch that referenced this pull request Nov 27, 2019
@pgomulka
[Java.time] Retain prefixed date pattern in formatter (elastic#48703)
fc1605d 
JavaDateFormatter should keep the pattern with the prefixed 8 as it will be used for serialisation.
The stripped pattern should be used for the enclosed formatters.

closes elastic#48698
@pgomulka pgomulka mentioned this pull request Nov 27, 2019
Closed
@rjernst
Copy link
Member

rjernst commented Nov 27, 2019

If this goes to master, what is the plan to remove it? We need a cutoff, which seemed like 7.x. At some point we need to convert these back to removing the 8 prefix, and between master and 7.x it should not be necessary since all formats are java 8 format.

@pgomulka
Copy link
Contributor Author

@rjernst you are right, this is not needed in master. When upgrading from 7.x to master users are not meant to have 8prefix in their formats.
In that case only 6.8 and 7.x should have this fix.

@pgomulka pgomulka mentioned this pull request Nov 28, 2019
Closed
@pgomulka pgomulka removed the v8.0.0 label Nov 29, 2019
@dliappis dliappis mentioned this pull request Nov 29, 2019
Closed
@pgomulka pgomulka mentioned this pull request Dec 11, 2019
Closed
@rjernst rjernst added v6.8.7 and removed v6.8.6 labels Dec 18, 2019
This was referenced Feb 3, 2020
Closed
Closed
@pgomulka pgomulka mentioned this pull request Jun 9, 2020
Closed
@pgomulka pgomulka mentioned this pull request Jun 14, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rjernst rjernst rjernst approved these changes

Assignees
No one assigned
Labels
>bug :Core/Infra/Core Core issues without another label v7.6.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@pgomulka @elasticmachine @rjernst @polyfractal @jaymode