Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use the same default time value for all replicas #10029

Merged
merged 1 commit into from
Dec 24, 2022

Conversation

Jackie-Jiang
Copy link
Contributor

When default time value is not provided, currently different replica of the segment will use different time (server local current time) as the default time value, which will cause inconsistency across replicas (each replica even have different crc, which cause unnecessary segment download during server restart).
This PR unifies the default time value to be the segment creation time from the ZK metadata.

Copy link
Contributor

@KKcorps KKcorps left a comment

Choose a reason for hiding this comment

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

LGTM!

@Jackie-Jiang Jackie-Jiang force-pushed the fix_default_time_value branch from 6614a27 to 699f70e Compare December 24, 2022 09:18
@codecov-commenter
Copy link

codecov-commenter commented Dec 24, 2022

Codecov Report

Merging #10029 (699f70e) into master (880a5c7) will increase coverage by 6.17%.
The diff coverage is 79.31%.

@@             Coverage Diff              @@
##             master   #10029      +/-   ##
============================================
+ Coverage     64.28%   70.46%   +6.17%     
- Complexity     5669     5695      +26     
============================================
  Files          1939     1994      +55     
  Lines        105033   107499    +2466     
  Branches      16041    16336     +295     
============================================
+ Hits          67518    75745    +8227     
+ Misses        32631    26471    -6160     
- Partials       4884     5283     +399     
Flag Coverage Δ
integration1 25.21% <79.31%> (?)
integration2 24.26% <75.86%> (?)
unittests1 67.98% <66.66%> (+0.02%) ⬆️
unittests2 13.59% <0.00%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
.../pinot/core/data/manager/BaseTableDataManager.java 74.65% <ø> (ø)
...ata/manager/realtime/RealtimeTableDataManager.java 68.70% <66.66%> (+39.42%) ⬆️
...server/starter/helix/HelixInstanceDataManager.java 71.48% <80.76%> (ø)
.../impl/dictionary/BaseOffHeapMutableDictionary.java 81.33% <0.00%> (-3.34%) ⬇️
...gregation/function/StUnionAggregationFunction.java 73.52% <0.00%> (-2.95%) ⬇️
...org/apache/pinot/core/common/ObjectSerDeUtils.java 90.61% <0.00%> (-0.77%) ⬇️
...pinot/server/starter/helix/HelixServerStarter.java 8.33% <0.00%> (ø)
.../starter/helix/HelixInstanceDataManagerConfig.java 79.59% <0.00%> (ø)
...che/pinot/server/api/resources/ShutDownFilter.java 40.00% <0.00%> (ø)
...t/server/api/resources/DefaultExceptionMapper.java 75.00% <0.00%> (ø)
... and 436 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Jackie-Jiang Jackie-Jiang merged commit ac5cd0b into apache:master Dec 24, 2022
@Jackie-Jiang Jackie-Jiang deleted the fix_default_time_value branch December 24, 2022 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants