Removed JodaTime dependency - migrated to java.time. #133
Conversation
Jonarzz
changed the title
Jonarzz
force-pushed
the
java_time_instead_of_joda
branch
from
4afde73
to
3940a8c
Compare
| @@ -34,19 +35,19 @@ | |||
| private final String type; | |||
| @JsonProperty("duration") | |||
| private long durationInMilliSeconds; | |||
| private DateTime origin; | |||
| private Temporal origin; | |||
There was a problem hiding this comment.
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 ?
| private DateTime startTime; | ||
| private DateTime endTime; | ||
| private TemporalAccessor startTime; | ||
| private TemporalAccessor endTime; |
There was a problem hiding this comment.
similar consideration here for startTime and endTime as for origin in DurationGranularity
| @@ -18,25 +18,6 @@ | |||
|
|
|||
| import com.fasterxml.jackson.core.JsonProcessingException; | |||
| import com.fasterxml.jackson.databind.ObjectMapper; | |||
|
|
|||
There was a problem hiding this comment.
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)
|
Hey, thanks for your contribution. Next steps :
|
abhi-zapr
added
the
breaking change
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. |
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 |
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. |
|
That seems reasonable. I'll apply a patch regarding your review in a few days, thanks! |
…ld cause errors in many cases as Temporal is too general); reverted code reordering (mostly imports).
| 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); |
There was a problem hiding this comment.
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.
| 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)); |
There was a problem hiding this comment.
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.
|
@abhi-zapr, is the v4 still planned for release? If so, I could take care of the comments you provided. |
resolves #130