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

Removed JodaTime dependency - migrated to java.time. #133

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Removed JodaTime dependency - migrated to java.time. #133

wants to merge 3 commits into from

Conversation

Jonarzz
Copy link
Contributor

@Jonarzz Jonarzz commented Feb 29, 2020 •
edited
Loading

resolves #130

@Jonarzz Jonarzz changed the title Removed JodaTime dependency - migrated to java.time (resolves #130). Removed JodaTime dependency - migrated to java.time. Feb 29, 2020
@Jonarzz Jonarzz force-pushed the java_time_instead_of_joda branch from 4afde73 to 3940a8c Compare February 29, 2020 14:36
abhi-zapr
abhi-zapr reviewed Mar 14, 2020
View reviewed changes
src/main/java/in/zapr/druid/druidry/granularity/DurationGranularity.java Outdated
@@ -34,19 +35,19 @@
private final String type;
@JsonProperty("duration")
private long durationInMilliSeconds;
private DateTime origin;
private Temporal origin;
Copy link
Contributor

Choose a reason for hiding this comment

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

If we keep origin as Temporal and user set it to Year, Instant, etc then wouldn't getOrigin fail with UnsupportedTemporalTypeException ? Shouldn't we keep origin as ZonedDateTime ?

src/main/java/in/zapr/druid/druidry/query/config/Interval.java Outdated
private DateTime startTime;
private DateTime endTime;
private TemporalAccessor startTime;
private TemporalAccessor endTime;
Copy link
Contributor

Choose a reason for hiding this comment

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

similar consideration here for startTime and endTime as for origin in DurationGranularity

src/test/java/in/zapr/druid/druidry/query/aggregation/GroupByTest.java
@@ -18,25 +18,6 @@

import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.ObjectMapper;

Copy link
Contributor

Choose a reason for hiding this comment

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

Too many unnecessary git diffs due to code rearrangement in many classes, can you fix these. (If you use repo style guide then this should be fixed)

@abhi-zapr
Copy link
Contributor

Hey, thanks for your contribution.
As this is a breaking change and an alternative as same thing is achievable through joda-time and incubator-druid also uses joda-time as of now, let's have a further discussion if this patch should be merged or not.

Next steps :

@abhi-zapr abhi-zapr added the breaking change patch is implementing new features but is breaking backward compatibility label Mar 14, 2020
@tunix
Copy link
Contributor

tunix commented Mar 17, 2020 •
edited
Loading

Hey, thanks for your contribution.
As this is a breaking change and an alternative as same thing is achievable through joda-time and incubator-druid also uses joda-time as of now, let's have a further discussion if this patch should be merged or not.

I think we wouldn't be affected by this change even if Druid itself continues with joda-time, would we? (afterall, it's a matter of serialization & deserialization?)

On the other hand, one expects to see java.time support since the library (also druid itself) is built on top of Java 8 and not any prior releases. Having to add joda-time to a project that fully adapted java.time package is a bit frustrating. It'd really be nice to see this change in the upstream.

@Jonarzz
Copy link
Contributor Author

Jonarzz commented Mar 18, 2020

On the other hand, one expects to see java.time support since the library (also druid itself) is built on top of Java 8 and not any prior releases. Having to add joda-time to a project that fully adapted java.time package is a bit frustrating

That's why in my opinion this change should be applied - Joda Time is just an unnecessary additional dependency.

Yet I do understand the hesitancy to merge this change as it would break all the code using Druidry and would require migration to java.time. Still - such change, if not required now (the code works fine as is and Joda API is all right), would make Druidry API better and the actual migration is not that hard (yet a docs update regarding that matter would be required).

@abhi-zapr
Copy link
Contributor

That's why in my opinion this change should be applied - Joda Time is just an unnecessary additional dependency.

Yes totally agree. But as it's breaking change, we could plan to release in next major release 4.0 just like 3.0 was breaking due to few patches.

@abhi-zapr abhi-zapr added this to the v4.0 milestone Mar 21, 2020
@Jonarzz
Copy link
Contributor Author

Jonarzz commented Mar 21, 2020

That seems reasonable. I'll apply a patch regarding your review in a few days, thanks!

Jonasz Czerepko added 2 commits April 27, 2020 17:08
Merge branch 'develop' into java_time_instead_of_joda
937e2da
Fixes after code review - ZonedDateTime used instead of Temporal (wou…
59ce930 
…ld cause errors in many cases as Temporal is too general); reverted code reordering (mostly imports).
abhi-zapr
abhi-zapr reviewed Jul 12, 2020
View reviewed changes
src/main/java/in/zapr/druid/druidry/granularity/DurationGranularity.java
this.type = DURATION_GRANULARITY_TYPE;
this.durationInMilliSeconds = durationInMilliSeconds;
this.origin = origin;
}

public String getOrigin() {
return origin == null ? null : origin.toDateTimeISO().toString();
return origin == null ? null : ISO_DATE_TIME.format(origin);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should use ISO_OFFSET_DATE_TIME formatter instead of ISO_DATE_TIME, as ISO_DATE_TIME formatter includes ZoneId along with offset.
Eg : Consider origin with ZoneId Asia/Kolkata
Then ISO_DATE_TIME.format(origin) will give you 2020-07-12T22:32:11.907+05:30[Asia/Kolkata]
vs origin.toDateTimeISO().toString() will give you 2020-07-12T22:32:11.907+05:30.
I think using ISO_DATE_TIME will break current contract.

src/main/java/in/zapr/druid/druidry/query/config/Interval.java
this.startTime = startTime;
this.endTime = endTime;
}

@JsonValue
private String getIntervalAsString() {
return String.format(DRUID_INTERVAL_FORMAT, startTime.toDateTimeISO(), endTime.toDateTimeISO());
return String.format(DRUID_INTERVAL_FORMAT, FORMATTER.format(startTime), FORMATTER.format(endTime));
Copy link
Contributor

Choose a reason for hiding this comment

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

Here also there will be contract issue and library might be giving wrong interval/s than what user intended as we are not including offset which was getting included before with toDateTimeISO().

Eg : Now getIntervalAsString() would be returning 2020-07-12T22:55:15.714Z/2020-07-12T22:55:15.714Z vs previous value 2020-07-12T22:55:15.714+05:30/2020-07-12T22:55:15.714+05:30 with offsets.

@Jonarzz
Copy link
Contributor Author

Jonarzz commented Aug 9, 2022

@abhi-zapr, is the v4 still planned for release? If so, I could take care of the comments you provided.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abhi-zapr abhi-zapr abhi-zapr left review comments

Assignees
No one assigned
Labels
breaking change patch is implementing new features but is breaking backward compatibility enhancement
Projects
None yet
Milestone
v4.0
Development

Successfully merging this pull request may close these issues.

Use Java 8's java.time module instead of joda
3 participants
@Jonarzz @abhi-zapr @tunix