Skip to content

Do not prefix already prefixed format with 8#48139

Merged
pgomulka merged 5 commits intoelastic:6.8from
pgomulka:fix/java-8-prefix-locale
Oct 17, 2019
Merged

Do not prefix already prefixed format with 8#48139
pgomulka merged 5 commits intoelastic:6.8from
pgomulka:fix/java-8-prefix-locale

Conversation

@pgomulka
Copy link
Copy Markdown
Contributor

@pgomulka pgomulka commented Oct 16, 2019

It happens when a locale is specified on a mapping that a new
JavaDateFormatter is created with a new mapping and already prefixed
with 8 format is prefixed again. This should change avoids this
behavior.

  • Have you signed the contributor license agreement?
  • Have you followed the contributor guidelines?
  • If submitting code, have you built your formula locally prior to submission with gradle check?
  • If submitting code, is your pull request against master? Unless there is a good reason otherwise, we prefer pull requests against master and will backport as needed.
  • If submitting code, have you checked that your submission is for an OS and architecture that we support?
  • If you are submitting this code for a class then read our policy for that.

@pgomulka
Do not prefix already prefixed format with 8
8406ca8 
It happens when a locale is specified on a mapping that a new
JavaDateFormatter is created with a new mapping and already prefixed
with 8 format is prefixed again. This should change avoids this
behavior.
@pgomulka pgomulka added :Core/Infra/Core Core issues without another label v6.8.5 labels Oct 16, 2019
@pgomulka pgomulka self-assigned this Oct 16, 2019
@elasticmachine
Copy link
Copy Markdown
Collaborator

elasticmachine commented Oct 16, 2019

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

@pgomulka
Copy link
Copy Markdown
Contributor Author

pgomulka commented Oct 16, 2019

discovered with yml test failure #47983

@pgomulka pgomulka mentioned this pull request Oct 16, 2019
Closed
@pgomulka
Copy link
Copy Markdown
Contributor Author

pgomulka commented Oct 16, 2019

@elasticmachine run elasticsearch-ci/packaging-sample-matrix

@pgomulka pgomulka requested a review from rjernst October 16, 2019 16:36
rjernst
rjernst approved these changes Oct 16, 2019
View reviewed changes
Copy link
Copy Markdown
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.

Looks fine. Just to double check: we would have never serialized multiple 8's right?

@pgomulka
Copy link
Copy Markdown
Contributor Author

pgomulka commented Oct 17, 2019

@elasticmachine run elasticsearch-ci/2

@pgomulka
Copy link
Copy Markdown
Contributor Author

pgomulka commented Oct 17, 2019

@rjernst good point, when serialising, it probably won't (I can't see how this could happen)
But when deserialising it would deserialise with double 8
added a testcase that fails without the change in JavaDateFormatter

@pgomulka pgomulka merged commit 1cc1885 into elastic:6.8 Oct 17, 2019
@jaymode jaymode added the >test Issues or PRs that are addressing/adding tests label Nov 19, 2019
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

:Core/Infra/Core Core issues without another label >test Issues or PRs that are addressing/adding tests v6.8.5

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@pgomulka @elasticmachine @rjernst @jaymode