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/15 13:28:50 UTC
[GitHub] [pinot] KKcorps opened a new pull request, #9403: Handle segments outside retention time gracefully during creation
KKcorps opened a new pull request, #9403:
URL: https://github.com/apache/pinot/pull/9403
Instructions:
1. The PR has to be tagged with at least one of the following labels (*):
1. `feature`
2. `bugfix`
3. `performance`
4. `ui`
5. `backward-incompat`
6. `release-notes` (**)
2. Remove these instructions before publishing the PR.
(*) Other labels to consider:
- `testing`
- `dependencies`
- `docker`
- `kubernetes`
- `observability`
- `security`
- `code-style`
- `extension-point`
- `refactor`
- `cleanup`
(**) Use `release-notes` label for scenarios like:
- New configuration options
- Deprecation of configurations
- Signature changes to public methods/interfaces
- New plugins added or old plugins removed
--
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 #9403: Handle segments outside retention time gracefully during creation
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #9403:
URL: https://github.com/apache/pinot/pull/9403#issuecomment-1252042240
# [Codecov](https://codecov.io/gh/apache/pinot/pull/9403?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 [#9403](https://codecov.io/gh/apache/pinot/pull/9403?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2c2bcce) into [master](https://codecov.io/gh/apache/pinot/commit/61fc9190bc1c2f15b3775acf892689a30da6df5e?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (61fc919) will **decrease** coverage by `35.12%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #9403 +/- ##
=============================================
- Coverage 69.77% 34.64% -35.13%
+ Complexity 5092 194 -4898
=============================================
Files 1890 1889 -1
Lines 100654 100870 +216
Branches 15327 15330 +3
=============================================
- Hits 70231 34948 -35283
- Misses 25445 62955 +37510
+ Partials 4978 2967 -2011
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `24.74% <0.00%> (-0.05%)` | :arrow_down: |
| unittests1 | `?` | |
| unittests2 | `15.35% <0.00%> (+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/9403?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ment/creator/impl/SegmentColumnarIndexCreator.java](https://codecov.io/gh/apache/pinot/pull/9403/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==) | `0.00% <0.00%> (-78.94%)` | :arrow_down: |
| [...in/java/org/apache/pinot/spi/utils/BytesUtils.java](https://codecov.io/gh/apache/pinot/pull/9403/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQnl0ZXNVdGlscy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../java/org/apache/pinot/spi/utils/BooleanUtils.java](https://codecov.io/gh/apache/pinot/pull/9403/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQm9vbGVhblV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...java/org/apache/pinot/spi/trace/BaseRecording.java](https://codecov.io/gh/apache/pinot/pull/9403/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdHJhY2UvQmFzZVJlY29yZGluZy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...java/org/apache/pinot/spi/trace/NoOpRecording.java](https://codecov.io/gh/apache/pinot/pull/9403/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdHJhY2UvTm9PcFJlY29yZGluZy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/config/table/FSTType.java](https://codecov.io/gh/apache/pinot/pull/9403/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3RhYmxlL0ZTVFR5cGUuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/config/user/RoleType.java](https://codecov.io/gh/apache/pinot/pull/9403/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3VzZXIvUm9sZVR5cGUuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/data/MetricFieldSpec.java](https://codecov.io/gh/apache/pinot/pull/9403/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9NZXRyaWNGaWVsZFNwZWMuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...java/org/apache/pinot/common/tier/TierFactory.java](https://codecov.io/gh/apache/pinot/pull/9403/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdGllci9UaWVyRmFjdG9yeS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...a/org/apache/pinot/spi/config/table/TableType.java](https://codecov.io/gh/apache/pinot/pull/9403/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-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3RhYmxlL1RhYmxlVHlwZS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [1115 more](https://codecov.io/gh/apache/pinot/pull/9403/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] KKcorps closed pull request #9403: Handle segments outside retention time gracefully during creation
Posted by GitBox <gi...@apache.org>.
KKcorps closed pull request #9403: Handle segments outside retention time gracefully during creation
URL: https://github.com/apache/pinot/pull/9403
--
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] KKcorps commented on pull request #9403: Handle segments outside retention time gracefully during creation
Posted by GitBox <gi...@apache.org>.
KKcorps commented on PR #9403:
URL: https://github.com/apache/pinot/pull/9403#issuecomment-1254623401
Yeah, I will go ahead with the idea of putting nulls in the metadata. This logic was created before we decided on `fixAPI`. Closing this PR.
--
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] KKcorps commented on pull request #9403: Handle segments outside retention time gracefully during creation
Posted by GitBox <gi...@apache.org>.
KKcorps commented on PR #9403:
URL: https://github.com/apache/pinot/pull/9403#issuecomment-1248110433
For now, only moved the code from previous PR (#9355) .
Last comments by @Jackie-Jiang
> We don't want to change the start/end time within the segment metadata, instead we want to enhance the time management code to gracefully handle the segments with invalid time values.
> Suggest using a separate PR for the time management change, and only keep the sample value check in this PR.
The new flag `skipSegmentTimeCheck: true` allows ingestion to not fail because of timestamp issues. As far as updating the timestamps in metadata is concerned, we need to find a way for users to specify the correct time-formats. The issue is we don't allow backward-incompatible change in the schema so if an user fixes the segment metadata timestamp via some new API, we won't be able to reflect those changes in the schema. This inconsistency may break a lot of things.
--
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] jackjlli commented on a diff in pull request #9403: Handle segments outside retention time gracefully during creation
Posted by GitBox <gi...@apache.org>.
jackjlli commented on code in PR #9403:
URL: https://github.com/apache/pinot/pull/9403#discussion_r972452979
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/SegmentColumnarIndexCreator.java:
##########
@@ -765,6 +766,28 @@ private void writeMetadata()
"Invalid segment start/end time: %s (in millis: %s/%s) for time column: %s, must be between: %s",
timeInterval, timeInterval.getStartMillis(), timeInterval.getEndMillis(), timeColumnName,
TimeUtils.VALID_TIME_INTERVAL);
+ } else {
+ Interval timeInterval =
+ new Interval(timeUnit.toMillis(startTime), timeUnit.toMillis(endTime), DateTimeZone.UTC);
+ long now = System.currentTimeMillis();
+ if (!TimeUtils.isValidTimeInterval(timeInterval)) {
+ startTime = now;
+ endTime = now;
+ }
+ SegmentsValidationAndRetentionConfig validationConfig = _config.getTableConfig().getValidationConfig();
+ String retentionTimeUnitConfig = validationConfig.getRetentionTimeUnit();
+ String retentionTimeValueConfig = validationConfig.getRetentionTimeValue();
+ try {
+ TimeUnit retentionTimeUnit = TimeUnit.valueOf(retentionTimeUnitConfig.toUpperCase());
+ long retentionTimeValue = Long.parseLong(retentionTimeValueConfig);
+ long retentionTimeMs = retentionTimeUnit.toMillis(retentionTimeValue);
+ if (endTime <= (now - retentionTimeMs)) {
Review Comment:
nit: put some comments to explicitly explain the rationale of doing this here.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/SegmentColumnarIndexCreator.java:
##########
@@ -765,6 +766,28 @@ private void writeMetadata()
"Invalid segment start/end time: %s (in millis: %s/%s) for time column: %s, must be between: %s",
timeInterval, timeInterval.getStartMillis(), timeInterval.getEndMillis(), timeColumnName,
TimeUtils.VALID_TIME_INTERVAL);
+ } else {
+ Interval timeInterval =
+ new Interval(timeUnit.toMillis(startTime), timeUnit.toMillis(endTime), DateTimeZone.UTC);
+ long now = System.currentTimeMillis();
+ if (!TimeUtils.isValidTimeInterval(timeInterval)) {
+ startTime = now;
+ endTime = now;
+ }
+ SegmentsValidationAndRetentionConfig validationConfig = _config.getTableConfig().getValidationConfig();
+ String retentionTimeUnitConfig = validationConfig.getRetentionTimeUnit();
+ String retentionTimeValueConfig = validationConfig.getRetentionTimeValue();
+ try {
+ TimeUnit retentionTimeUnit = TimeUnit.valueOf(retentionTimeUnitConfig.toUpperCase());
+ long retentionTimeValue = Long.parseLong(retentionTimeValueConfig);
+ long retentionTimeMs = retentionTimeUnit.toMillis(retentionTimeValue);
+ if (endTime <= (now - retentionTimeMs)) {
+ endTime = now;
+ }
+ } catch (Exception e) {
+ LOGGER.warn("Invalid retention time: {} {} for table: {}, skip", retentionTimeUnitConfig,
Review Comment:
nit: it'd be good to state what step is skipped here.
--
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