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 2022/09/30 11:42:20 UTC
[GitHub] [pinot] KKcorps opened a new pull request, #9505: Allow batch segments to have invalid start/end time
KKcorps opened a new pull request, #9505:
URL: https://github.com/apache/pinot/pull/9505
Currently, we simply fail the batch ingestion process if segments if start/end time is outside of time-range or values are not parsed correctly. This is not needed now since we have a new API from PR #9413
So users should be able to ingest segments and fix their time interval later on.
This behaviour is disabled by default and can only be switched with `continueOnError: true` flag for a table.
This is being done in the following way -
* If there is an error in parsing time values during Segment creation for start/end time, we simply don't store the START/END time in the metadata.
* The ZkMetadata simply returns -1 as start/end time in this case.
* All the places which call `SegmentZKMetadata.getStartTimeMs` or `SegmentZKMetadata.getEndTimeMs` handle the negative timestamps. Either an exception is thrown that is already handled (e.g. TimeBasedTierSegmentSelector.selectSegment` OR we ignore the segments (e.g. `MergeRollupTask`)
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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] [pinot] codecov-commenter commented on pull request #9505: Allow batch segments to have invalid start/end time
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #9505:
URL: https://github.com/apache/pinot/pull/9505#issuecomment-1264078525
# [Codecov](https://codecov.io/gh/apache/pinot/pull/9505?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#9505](https://codecov.io/gh/apache/pinot/pull/9505?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (0502bda) into [master](https://codecov.io/gh/apache/pinot/commit/b026d321d335b0e452010bdd51ff9b09ac6f55d2?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b026d32) will **decrease** coverage by `6.23%`.
> The diff coverage is `48.88%`.
```diff
@@ Coverage Diff @@
## master #9505 +/- ##
============================================
- Coverage 70.00% 63.77% -6.24%
- Complexity 4792 4847 +55
============================================
Files 1921 1868 -53
Lines 102349 100104 -2245
Branches 15530 15275 -255
============================================
- Hits 71651 63838 -7813
- Misses 25636 31564 +5928
+ Partials 5062 4702 -360
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `?` | |
| unittests1 | `67.29% <55.17%> (-0.02%)` | :arrow_down: |
| unittests2 | `15.61% <13.33%> (+0.02%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/9505?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...pinot/controller/util/TableRetentionValidator.java](https://codecov.io/gh/apache/pinot/pull/9505/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci91dGlsL1RhYmxlUmV0ZW50aW9uVmFsaWRhdG9yLmphdmE=) | `0.00% <0.00%> (ø)` | |
| [...on/tasks/mergerollup/MergeRollupTaskGenerator.java](https://codecov.io/gh/apache/pinot/pull/9505/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcGx1Z2lucy9waW5vdC1taW5pb24tdGFza3MvcGlub3QtbWluaW9uLWJ1aWx0aW4tdGFza3Mvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3BsdWdpbi9taW5pb24vdGFza3MvbWVyZ2Vyb2xsdXAvTWVyZ2VSb2xsdXBUYXNrR2VuZXJhdG9yLmphdmE=) | `74.76% <0.00%> (-10.10%)` | :arrow_down: |
| [...ot/controller/helix/core/util/ZKMetadataUtils.java](https://codecov.io/gh/apache/pinot/pull/9505/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL3V0aWwvWktNZXRhZGF0YVV0aWxzLmphdmE=) | `80.88% <40.00%> (-11.31%)` | :arrow_down: |
| [...ment/creator/impl/SegmentColumnarIndexCreator.java](https://codecov.io/gh/apache/pinot/pull/9505/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2NyZWF0b3IvaW1wbC9TZWdtZW50Q29sdW1uYXJJbmRleENyZWF0b3IuamF2YQ==) | `77.21% <55.17%> (-1.73%)` | :arrow_down: |
| [...gments/RealtimeToOfflineSegmentsTaskGenerator.java](https://codecov.io/gh/apache/pinot/pull/9505/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcGx1Z2lucy9waW5vdC1taW5pb24tdGFza3MvcGlub3QtbWluaW9uLWJ1aWx0aW4tdGFza3Mvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3BsdWdpbi9taW5pb24vdGFza3MvcmVhbHRpbWV0b29mZmxpbmVzZWdtZW50cy9SZWFsdGltZVRvT2ZmbGluZVNlZ21lbnRzVGFza0dlbmVyYXRvci5qYXZh) | `92.75% <66.66%> (-2.10%)` | :arrow_down: |
| [...va/org/apache/pinot/core/routing/RoutingTable.java](https://codecov.io/gh/apache/pinot/pull/9505/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9yb3V0aW5nL1JvdXRpbmdUYWJsZS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...va/org/apache/pinot/common/config/NettyConfig.java](https://codecov.io/gh/apache/pinot/pull/9505/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vY29uZmlnL05ldHR5Q29uZmlnLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...a/org/apache/pinot/common/metrics/MinionMeter.java](https://codecov.io/gh/apache/pinot/pull/9505/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9NaW5pb25NZXRlci5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...g/apache/pinot/common/metrics/ControllerMeter.java](https://codecov.io/gh/apache/pinot/pull/9505/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9Db250cm9sbGVyTWV0ZXIuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../apache/pinot/common/metrics/BrokerQueryPhase.java](https://codecov.io/gh/apache/pinot/pull/9505/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9Ccm9rZXJRdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [447 more](https://codecov.io/gh/apache/pinot/pull/9505/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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] [pinot] Jackie-Jiang commented on a diff in pull request #9505: Allow batch segments to have invalid start/end time
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on code in PR #9505:
URL: https://github.com/apache/pinot/pull/9505#discussion_r995155971
##########
pinot-controller/src/test/java/org/apache/pinot/controller/utils/SegmentMetadataMockUtils.java:
##########
@@ -46,12 +46,13 @@ public static SegmentMetadata mockSegmentMetadata(String tableName, String segme
Mockito.when(segmentMetadata.getName()).thenReturn(segmentName);
Mockito.when(segmentMetadata.getTotalDocs()).thenReturn(numTotalDocs);
Mockito.when(segmentMetadata.getCrc()).thenReturn(crc);
- Mockito.when(segmentMetadata.getStartTime()).thenReturn(1L);
- Mockito.when(segmentMetadata.getEndTime()).thenReturn(10L);
+ Mockito.when(segmentMetadata.getTimeColumn()).thenReturn(tableName);
Review Comment:
(minor) Put a dummy time column name such as `"timeCol"`, same for line 105
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/SegmentColumnarIndexCreator.java:
##########
@@ -758,18 +768,20 @@ private void writeMetadata()
}
}
+ Interval timeInterval =
+ new Interval(timeUnit.toMillis(startTime), timeUnit.toMillis(endTime), DateTimeZone.UTC);
+ boolean isValidTimeInterval = TimeUtils.isValidTimeInterval(timeInterval);
if (!_config.isSkipTimeValueCheck()) {
Review Comment:
(minor) Suggest changing the check order a little bit and log a warning when time interval is not valid, but time value check is skipped
```suggestion
if (isValidTimeInterval) {
setSegmentTimeInterval(properties, startTime, endTime, timeUnit);
} else {
if (_config.isSkipTimeValueCheck()) {
LOGGER.warn(...);
} else {
throw new IllegalStateException(...);
}
}
```
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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