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

Deserialize timeZone with DateUtils in RangeQueryBuilder #38449

Closed
pgomulka opened this issue Feb 5, 2019 · 10 comments · May be fixed by #104593
Closed

Deserialize timeZone with DateUtils in RangeQueryBuilder #38449

pgomulka opened this issue Feb 5, 2019 · 10 comments · May be fixed by #104593
Labels
:Core/Infra/Core Core issues without another label good first issue low hanging fruit Team:Core/Infra Meta label for core/infra team

Comments

@pgomulka
Copy link
Contributor

pgomulka commented Feb 5, 2019

as spotted in this review #38415 (comment)
DateUtil should be used when serialising/deserializing timezones to allow deprecation logging

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

Pinging @elastic/es-core-infra

@rjernst rjernst added the Team:Core/Infra Meta label for core/infra team label May 4, 2020
@rjernst rjernst added the needs:triage Requires assignment of a team area label label Dec 3, 2020
@gwbrown
Copy link
Contributor

gwbrown commented Dec 15, 2020

Looks like the place that actually needs to be changed to use DateUtils.dateTimeZoneToZoneId is:

@gwbrown gwbrown added good first issue low hanging fruit and removed needs:triage Requires assignment of a team area label labels Dec 15, 2020
pgomulka added a commit to pgomulka/elasticsearch that referenced this issue Dec 30, 2020
DateUtils.of methods allows to emitt deprecation message when a
deprecated timezone was used

closes elastic#38449
@pedrochamberlain
Copy link

Hello, is this issue still open? I'd like to give it a shot. This would be my first issue.

@LeeTgk
Copy link

LeeTgk commented May 26, 2022

Hi. is this issue still open? I would like to try it as my first issue. 🤨:mag:

@pgomulka
Copy link
Contributor Author

@LeeTgk sure, it is still open. Feel free to send PR

@sojuforme
Copy link

I attempted to fix this issue. I created a pull request and tried to make the commit as clear as possible.

@timothy-choi
Copy link

Hello is this issue still open? I would like to fix this issue.

@happyCupcake
Copy link

happyCupcake commented Feb 18, 2024

@gwbrown @williamrandolph @pgomulka I am new to open source and to elasticsearch and with some guidance I can help. I reviewed all comments and previous PR's.

Here, the call in.readOptionalZoneId() needs to be replaced with DateUtils.dateTimeZoneToZoneId(DateTimeZone) , but its not clear to me how because dateTimeZoneToZoneId takes as input timezone and returns zone id but the current code is reading zoneId and storing as timezone.

Does the stream contain a zoneId or timezone ? If its zoneId, then we dont need to call dateTimeZoneToZoneId, but if the stream contains timezone, then I need to convert string timezone to DateTimeZone before calling dateTimeZoneToZoneId

Any guidance will help :-)

public RangeQueryBuilder(StreamInput in) throws IOException {
..

timeZone = in.readOptionalZoneId();

@ayushrawat99
Copy link

HI I am new and want to give it a shot. Can I pick this up ?

@ldematte
Copy link
Contributor

ldematte commented Dec 6, 2024

Not needed anymore since #40647

@ldematte ldematte closed this as completed Dec 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Core Core issues without another label good first issue low hanging fruit Team:Core/Infra Meta label for core/infra team
Projects
None yet