Skip to content

check for less than zero start and end time#7372

Merged
jackjlli merged 1 commit intoapache:masterfrom
jasperjiaguo:SegmentZKMetadata_Time_Check
Aug 27, 2021
Merged

check for less than zero start and end time#7372
jackjlli merged 1 commit intoapache:masterfrom
jasperjiaguo:SegmentZKMetadata_Time_Check

Conversation

@jasperjiaguo
Copy link
Contributor

@jasperjiaguo jasperjiaguo commented Aug 27, 2021

Description

Check for the "-1" start time in some use cases

Upgrade Notes

Does this PR prevent a a zero-downtime upgrade?
No

Does this PR fix a zero-downtime upgrade introduced earlier?
No

Copy link
Contributor

@snleee snleee left a comment

Choose a reason for hiding this comment

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

LGTM

@codecov-commenter
Copy link

codecov-commenter commented Aug 27, 2021

Codecov Report

Merging #7372 (c208e89) into master (21767c6) will decrease coverage by 38.25%.
The diff coverage is 0.00%.

❗ Current head c208e89 differs from pull request most recent head 0c8d0e1. Consider uploading reports for the commit 0c8d0e1 to get more accurate results
Impacted file tree graph

@@              Coverage Diff              @@
##             master    #7372       +/-   ##
=============================================
- Coverage     70.40%   32.14%   -38.26%     
+ Complexity     3307       93     -3214     
=============================================
  Files          1515     1506        -9     
  Lines         75008    74660      -348     
  Branches      10910    10872       -38     
=============================================
- Hits          52808    24000    -28808     
- Misses        18572    48583    +30011     
+ Partials       3628     2077     -1551     
Flag Coverage Δ
integration1 30.48% <0.00%> (?)
integration2 28.99% <0.00%> (-0.06%) ⬇️
unittests1 ?
unittests2 ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...not/common/metadata/segment/SegmentZKMetadata.java 68.48% <0.00%> (-3.64%) ⬇️
...c/main/java/org/apache/pinot/common/tier/Tier.java 0.00% <0.00%> (-100.00%) ⬇️
.../java/org/apache/pinot/spi/utils/BooleanUtils.java 0.00% <0.00%> (-100.00%) ⬇️
...ava/org/apache/pinot/spi/data/MetricFieldSpec.java 0.00% <0.00%> (-100.00%) ⬇️
...va/org/apache/pinot/spi/utils/BigDecimalUtils.java 0.00% <0.00%> (-100.00%) ⬇️
...java/org/apache/pinot/common/tier/TierFactory.java 0.00% <0.00%> (-100.00%) ⬇️
...a/org/apache/pinot/spi/config/table/TableType.java 0.00% <0.00%> (-100.00%) ⬇️
.../org/apache/pinot/spi/data/DimensionFieldSpec.java 0.00% <0.00%> (-100.00%) ⬇️
.../org/apache/pinot/spi/data/readers/FileFormat.java 0.00% <0.00%> (-100.00%) ⬇️
...org/apache/pinot/spi/config/table/QuotaConfig.java 0.00% <0.00%> (-100.00%) ⬇️
... and 1041 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 21767c6...0c8d0e1. Read the comment docs.

@jackjlli jackjlli merged commit 2302bd2 into apache:master Aug 27, 2021
@mcvsubbu
Copy link
Contributor

What broke because of this? Was this something that happened due to the recent re-factor of metadata?

@jasperjiaguo
Copy link
Contributor Author

What broke because of this? Was this something that happened due to the recent re-factor of metadata?

Due to the previous change in #7255 didn't check for negative start/end time.

@mcvsubbu
Copy link
Contributor

OK, so the periodic tasks start to throw exception on refresh tables and we see exception logs in the controller because of PR #7255

jasperjiaguo added a commit to jasperjiaguo/incubator-pinot that referenced this pull request Aug 27, 2021
@Jackie-Jiang
Copy link
Contributor

Is the issue caused by some old segment ZK metadata set negative start/end time and null time unit?

@Jackie-Jiang
Copy link
Contributor

getTimeInterval() also needs to be fixed. Please take a look at #7375

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants