You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2020/11/04 07:59:23 UTC
[GitHub] [incubator-pinot] hotienvu opened a new pull request #6233: Make sure DatetimeFormatter always uses UTC
hotienvu opened a new pull request #6233:
URL: https://github.com/apache/incubator-pinot/pull/6233
## Description
This PR fixes the bug when not running in UTC timezone, DateTimeFormatter.parseMillis will return wrong result.
e.g.
```
DateTimeFormat.forPattern("yyyyMMdd").parseMillis("19710101") -> 31509000000 (December 31, 1970 4:30:00 PM) -> wrong
DateTimeFormat.forPattern("yyyyMMdd").withZoneUTC().parseMillis("19710101") -> 31536000000 (January 1, 1971 12:00:00 AM) -> correct
```
## Upgrade Notes
Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)
* [ ] Yes (Please label as **<code>backward-incompat</code>**, and complete the section below on Release Notes)
Does this PR fix a zero-downtime upgrade introduced earlier?
* [ ] Yes (Please label this as **<code>backward-incompat</code>**, and complete the section below on Release Notes)
Does this PR otherwise need attention when creating release notes? Things to consider:
- New configuration options
- Deprecation of configurations
- Signature changes to public methods/interfaces
- New plugins added or old plugins removed
* [ ] Yes (Please label this PR as **<code>release-notes</code>** and complete the section on Release Notes)
## Release Notes
If you have tagged this as either backward-incompat or release-notes,
you MUST add text here that you would like to see appear in release notes of the
next release.
If you have a series of commits adding or enabling a feature, then
add this section only in final commit that marks the feature completed.
Refer to earlier release notes to see examples of text
## Documentation
If you have introduced a new feature or configuration, please add it to the documentation as well.
See https://docs.pinot.apache.org/developers/developers-and-contributors/update-document
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] hotienvu closed pull request #6233: Make sure DatetimeFormatter always uses UTC
Posted by GitBox <gi...@apache.org>.
hotienvu closed pull request #6233:
URL: https://github.com/apache/incubator-pinot/pull/6233
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] hotienvu commented on pull request #6233: Make sure DatetimeFormatter always uses UTC
Posted by GitBox <gi...@apache.org>.
hotienvu commented on pull request #6233:
URL: https://github.com/apache/incubator-pinot/pull/6233#issuecomment-722234395
@Jackie-Jiang Actually when I tried to run the test in my local machine, this is what it gives: (my local timezone is UTC+8)
```
java.lang.IllegalStateException: Invalid segment start/end time: 1970-12-31T16:30:00.000Z/2020-10-29T16:00:00.000Z (in millis: 31509000000/1603987200000) for time column: date, must be between: 1971-01-01T00:00:00.000Z/2071-01-01T00:00:00.000Z
at com.google.common.base.Preconditions.checkState(Preconditions.java:469)
at org.apache.pinot.core.segment.creator.impl.SegmentColumnarIndexCreator.writeMetadata(SegmentColumnarIndexCreator.java:457)
at org.apache.pinot.core.segment.creator.impl.SegmentColumnarIndexCreator.seal(SegmentColumnarIndexCreator.java:404)
at org.apache.pinot.core.segment.creator.impl.SegmentIndexCreationDriverImpl.handlePostCreation(SegmentIndexCreationDriverImpl.java:252)
at org.apache.pinot.core.segment.creator.impl.SegmentIndexCreationDriverImpl.build(SegmentIndexCreationDriverImpl.java:235)
at org.apache.pinot.core.segment.index.creator.SegmentGenerationWithTimeColumnTest.buildSegment(SegmentGenerationWithTimeColumnTest.java:205)
at org.apache.pinot.core.segment.index.creator.SegmentGenerationWithTimeColumnTest.testSimpleDateSegmentGeneration(SegmentGenerationWithTimeColumnTest.java:91)
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on pull request #6233: Make sure DatetimeFormatter always uses UTC
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on pull request #6233:
URL: https://github.com/apache/incubator-pinot/pull/6233#issuecomment-722042464
I'm not sure if this is the desired behavior, and if this can cause backward-incompatible issue. Some use cases might require to parse the time string with the local time zone.
What specific issue are you running into because of this?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] hotienvu commented on pull request #6233: Make sure DatetimeFormatter always uses UTC
Posted by GitBox <gi...@apache.org>.
hotienvu commented on pull request #6233:
URL: https://github.com/apache/incubator-pinot/pull/6233#issuecomment-722239601
Since the output we want is millis since epoch, this would still work regardless of whether user specify a time zone or not.
```
DateTimeFormat.forPattern("yyyyMMdd HH:mm:ss Z").withZoneUTC().parseMillis("19710101 00:00:00 +0800") -> 31507200000
DateTimeFormat.forPattern("yyyyMMdd HH:mm:ss Z").parseMillis("19710101 00:00:00 +0800") -> 31507200000
```
However if we don't specify time timezone and our locale is not UTC e.g +0800
```
DateTimeFormat.forPattern("yyyyMMdd HH:mm:ss").parseMillis("19710101 00:00:00") -> 31509000000
DateTimeFormat.forPattern("yyyyMMdd HH:mm:ss").withZoneUTC().parseMillis("19710101 00:00:00") -> 31536000000
```
So I think it's better to enforce UTC in this case
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] hotienvu commented on pull request #6233: Make sure DatetimeFormatter always uses UTC
Posted by GitBox <gi...@apache.org>.
hotienvu commented on pull request #6233:
URL: https://github.com/apache/incubator-pinot/pull/6233#issuecomment-722248030
> Seems this change only impact segment name generation, does it also impact the query results?
@fx19880617 Please see example above, the output of `dateTimeFormatter.parseMillis` should be the same, so no change to output. This is just to handle the corner case when the os timezone is not UTC.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] codecov-io commented on pull request #6233: Make sure DatetimeFormatter always uses UTC
Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #6233:
URL: https://github.com/apache/incubator-pinot/pull/6233#issuecomment-721592862
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6233?src=pr&el=h1) Report
> Merging [#6233](https://codecov.io/gh/apache/incubator-pinot/pull/6233?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) will **decrease** coverage by `2.34%`.
> The diff coverage is `43.43%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6233?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6233 +/- ##
==========================================
- Coverage 66.44% 64.10% -2.35%
==========================================
Files 1075 1240 +165
Lines 54773 58938 +4165
Branches 8168 8737 +569
==========================================
+ Hits 36396 37783 +1387
- Misses 15700 18404 +2704
- Partials 2677 2751 +74
```
| Flag | Coverage Δ | |
|---|---|---|
| unittests | `64.10% <43.43%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6233?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...e/pinot/broker/api/resources/PinotBrokerDebug.java](https://codecov.io/gh/apache/incubator-pinot/pull/6233/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYXBpL3Jlc291cmNlcy9QaW5vdEJyb2tlckRlYnVnLmphdmE=) | `0.00% <0.00%> (-79.32%)` | :arrow_down: |
| [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6233/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `71.42% <ø> (-28.58%)` | :arrow_down: |
| [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6233/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclVzZXJEZWZpbmVkTWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `33.96% <0.00%> (-32.71%)` | :arrow_down: |
| [...ker/routing/instanceselector/InstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6233/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...ava/org/apache/pinot/client/AbstractResultSet.java](https://codecov.io/gh/apache/incubator-pinot/pull/6233/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Fic3RyYWN0UmVzdWx0U2V0LmphdmE=) | `66.66% <0.00%> (+9.52%)` | :arrow_up: |
| [.../main/java/org/apache/pinot/client/Connection.java](https://codecov.io/gh/apache/incubator-pinot/pull/6233/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Nvbm5lY3Rpb24uamF2YQ==) | `35.55% <0.00%> (-13.29%)` | :arrow_down: |
| [...inot/client/JsonAsyncHttpPinotClientTransport.java](https://codecov.io/gh/apache/incubator-pinot/pull/6233/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0pzb25Bc3luY0h0dHBQaW5vdENsaWVudFRyYW5zcG9ydC5qYXZh) | `10.90% <0.00%> (-51.10%)` | :arrow_down: |
| [...not/common/assignment/InstancePartitionsUtils.java](https://codecov.io/gh/apache/incubator-pinot/pull/6233/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vYXNzaWdubWVudC9JbnN0YW5jZVBhcnRpdGlvbnNVdGlscy5qYXZh) | `73.80% <ø> (+0.63%)` | :arrow_up: |
| [.../apache/pinot/common/exception/QueryException.java](https://codecov.io/gh/apache/incubator-pinot/pull/6233/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZXhjZXB0aW9uL1F1ZXJ5RXhjZXB0aW9uLmphdmE=) | `90.27% <ø> (+5.55%)` | :arrow_up: |
| [...pinot/common/function/AggregationFunctionType.java](https://codecov.io/gh/apache/incubator-pinot/pull/6233/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vQWdncmVnYXRpb25GdW5jdGlvblR5cGUuamF2YQ==) | `98.27% <ø> (-1.73%)` | :arrow_down: |
| ... and [1078 more](https://codecov.io/gh/apache/incubator-pinot/pull/6233/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6233?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6233?src=pr&el=footer). Last update [bee125e...4aa2af2](https://codecov.io/gh/apache/incubator-pinot/pull/6233?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on pull request #6233: Make sure DatetimeFormatter always uses UTC
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on pull request #6233:
URL: https://github.com/apache/incubator-pinot/pull/6233#issuecomment-722517538
Seems like better to change the test, since there may be backward incompatibility introduced.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] fx19880617 commented on pull request #6233: Make sure DatetimeFormatter always uses UTC
Posted by GitBox <gi...@apache.org>.
fx19880617 commented on pull request #6233:
URL: https://github.com/apache/incubator-pinot/pull/6233#issuecomment-721939287
Seems this change only impact segment name generation, does it also impact the query results?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org