Add a new API to fix segment date time in metadata #9413

KKcorps · 2022-09-16T16:42:14Z

Currently, if an user specifies an incorrect datetime format accidentally, we don't allow any way to fix the segment start and end time in zookeeper metadata.
This creates problem when the end times lie before the retention interval in which case those segments get removed. It also creates problem in case of time pruning during queries.

The PR aims to introduce a new API to address this issue. It is still Work in progress.

pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/util/ZKMetadataUtils.java

pinot-spi/src/main/java/org/apache/pinot/spi/data/Schema.java

...in/java/org/apache/pinot/segment/local/segment/creator/impl/SegmentColumnarIndexCreator.java

pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/SegmentMetadata.java

pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/util/ZKMetadataUtils.java

...ntroller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java

Jackie-Jiang · 2022-09-21T20:49:43Z

...ntroller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java

Throw exception when table config is not available

...ntroller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java

...ller/src/main/java/org/apache/pinot/controller/api/resources/PinotSchemaRestletResource.java

codecov-commenter · 2022-09-23T11:07:40Z

Codecov Report

Merging #9413 (90adbff) into master (69722b0) will decrease coverage by 34.58%.
The diff coverage is 48.33%.

@@              Coverage Diff              @@
##             master    #9413       +/-   ##
=============================================
- Coverage     69.83%   35.25%   -34.59%     
+ Complexity     4718      189     -4529     
=============================================
  Files          1902     1915       +13     
  Lines        101521   102049      +528     
  Branches      15411    15479       +68     
=============================================
- Hits          70899    35973    -34926     
- Misses        25623    63022    +37399     
+ Partials       4999     3054     -1945

Flag	Coverage Δ
integration1	`26.13% <20.00%> (+0.13%)`	⬆️
integration2	`?`
unittests1	`?`
unittests2	`15.52% <38.33%> (+0.05%)`	⬆️

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

Impacted Files	Coverage Δ
...ler/api/resources/PinotSegmentRestletResource.java	`20.00% <0.00%> (-18.71%)`	⬇️
...va/org/apache/pinot/spi/utils/CommonConstants.java	`0.00% <ø> (-25.00%)`	⬇️
...not/common/metadata/segment/SegmentZKMetadata.java	`81.04% <66.66%> (-5.35%)`	⬇️
...ot/controller/helix/core/util/ZKMetadataUtils.java	`87.87% <75.00%> (-10.04%)`	⬇️
...ntroller/helix/core/PinotHelixResourceManager.java	`70.05% <100.00%> (-1.55%)`	⬇️
...in/java/org/apache/pinot/spi/utils/BytesUtils.java	`0.00% <0.00%> (-100.00%)`	⬇️
...java/org/apache/pinot/spi/trace/BaseRecording.java	`0.00% <0.00%> (-100.00%)`	⬇️
...java/org/apache/pinot/spi/trace/NoOpRecording.java	`0.00% <0.00%> (-100.00%)`	⬇️
...ava/org/apache/pinot/spi/config/table/FSTType.java	`0.00% <0.00%> (-100.00%)`	⬇️
...ava/org/apache/pinot/spi/config/user/RoleType.java	`0.00% <0.00%> (-100.00%)`	⬇️
... and 1151 more

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

...ler/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentRestletResource.java

pinot-spi/src/main/java/org/apache/pinot/spi/data/Schema.java

...gment-spi/src/main/java/org/apache/pinot/segment/spi/index/metadata/SegmentMetadataImpl.java

...in/java/org/apache/pinot/segment/local/segment/creator/impl/SegmentColumnarIndexCreator.java

...ler/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentRestletResource.java

...ntroller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java

pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/util/ZKMetadataUtils.java

Jackie-Jiang · 2022-09-26T19:18:02Z

pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/util/ZKMetadataUtils.java

    if (segmentMetadata.getTimeInterval() != null) {
      segmentZKMetadata.setStartTime(segmentMetadata.getStartTime());
      segmentZKMetadata.setEndTime(segmentMetadata.getEndTime());
      segmentZKMetadata.setTimeUnit(segmentMetadata.getTimeUnit());


We can change this to always put MILLISECONDS as the time unit (convert start/end time using the time unit), then we no longer need to worry about time unit when updating the time interval

One reason why I'm suggesting this is that there is one corner case: if the original date time format is in SECONDS, then changed to MILLISECONDS, if we keep the time unit as the old one, we might lose precision

Makes sense. Closing the PR #9472. I haven't removed the logic to convert timestamp using timeUnit since I am not sure about backward compatibility

...ler/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentRestletResource.java

Jackie-Jiang · 2022-09-28T19:57:22Z

pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/util/ZKMetadataUtils.java

-      segmentZKMetadata.setStartTime(segmentMetadata.getStartTime());
-      segmentZKMetadata.setEndTime(segmentMetadata.getEndTime());
-      segmentZKMetadata.setTimeUnit(segmentMetadata.getTimeUnit());
+      long startTimeMs = TimeUnit.MILLISECONDS.convert(segmentMetadata.getStartTime(), segmentMetadata.getTimeUnit());


(minor) start/end time can be easily read from the time interval without converting again

#9413 (comment)

Jackie-Jiang

LGTM otherwise

...ler/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentRestletResource.java

...ntroller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java

...ler/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentRestletResource.java

* Add API to handle change in timestamp format * Fix linting * Add support for storing raw segment start and end time in metadata * Do not store start end time in segment metadata seperately * remove refresh flag * Store new start/end time with proper time unit * Move the API to segment resource * Fix test failure * Add test * Cleanup: Remove duplicate methods * Store start/end time in milliseconds in zookeeper * Refactor: change method names and reduce scope of exceptions * Remove redundant timeunit conversion * Throw user errors and check for time column * Fix Segment tests Co-authored-by: Kartik Khare <[email protected]>

KKcorps force-pushed the fix_segment_datetime_api branch from 8957120 to 8b1088e Compare September 16, 2022 16:45

Jackie-Jiang reviewed Sep 16, 2022

View reviewed changes

pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/util/ZKMetadataUtils.java Outdated Show resolved Hide resolved

KKcorps force-pushed the fix_segment_datetime_api branch from c6a638f to 3db9e6c Compare September 20, 2022 10:40

Jackie-Jiang reviewed Sep 21, 2022

View reviewed changes

Kartik Khare added 4 commits September 22, 2022 13:04

Add API to handle change in timestamp format

c614e5f

Fix linting

0cb0f59

Add support for storing raw segment start and end time in metadata

229cade

Do not store start end time in segment metadata seperately

466395b

KKcorps force-pushed the fix_segment_datetime_api branch from 3db9e6c to 466395b Compare September 22, 2022 11:19

Kartik Khare added 3 commits September 22, 2022 17:19

remove refresh flag

77c63ef

Store new start/end time with proper time unit

4094ee4

Move the API to segment resource

3223389

KKcorps changed the title ~~[WIP] Add a new API to fix segment date time in metadata~~ Add a new API to fix segment date time in metadata Sep 23, 2022

KKcorps force-pushed the fix_segment_datetime_api branch 2 times, most recently from ed749e4 to 9650f8e Compare September 25, 2022 03:57

Fix test failure

99ef712

KKcorps force-pushed the fix_segment_datetime_api branch from 9650f8e to 99ef712 Compare September 25, 2022 08:48

KKcorps requested a review from Jackie-Jiang September 26, 2022 05:17

Add test

ced29b1

Jackie-Jiang reviewed Sep 26, 2022

View reviewed changes

KKcorps force-pushed the fix_segment_datetime_api branch from b4b9d7a to 440f6d3 Compare September 27, 2022 06:05

Cleanup: Remove duplicate methods

f9e5bb1

KKcorps force-pushed the fix_segment_datetime_api branch from 440f6d3 to f9e5bb1 Compare September 27, 2022 06:32

Jackie-Jiang added feature release-notes Referenced by PRs that need attention when compiling the next release notes labels Sep 28, 2022

Jackie-Jiang reviewed Sep 28, 2022

View reviewed changes

...ler/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentRestletResource.java Outdated Show resolved Hide resolved

Store start/end time in milliseconds in zookeeper

965d857

KKcorps force-pushed the fix_segment_datetime_api branch from 9020520 to 965d857 Compare September 28, 2022 05:45

KKcorps requested a review from Jackie-Jiang September 28, 2022 19:17

Jackie-Jiang reviewed Sep 28, 2022

View reviewed changes

Kartik Khare added 2 commits September 29, 2022 01:46

Refactor: change method names and reduce scope of exceptions

f7782fc

Remove redundant timeunit conversion

cf42232

KKcorps force-pushed the fix_segment_datetime_api branch from 952d495 to cf42232 Compare September 29, 2022 00:27

Jackie-Jiang approved these changes Sep 29, 2022

View reviewed changes

Throw user errors and check for time column

4d335f5

KKcorps force-pushed the fix_segment_datetime_api branch from 90adbff to 4d335f5 Compare September 29, 2022 02:51

Fix Segment tests

5427fa8

KKcorps force-pushed the fix_segment_datetime_api branch from e56cf26 to 5427fa8 Compare September 29, 2022 09:52

KKcorps merged commit d2619c1 into apache:master Sep 29, 2022

KKcorps mentioned this pull request Sep 30, 2022

Allow batch segments to have invalid start/end time #9505

Open

Add a new API to fix segment date time in metadata #9413

Add a new API to fix segment date time in metadata #9413

Uh oh!

Conversation

KKcorps commented Sep 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Jackie-Jiang Sep 21, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented Sep 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Jackie-Jiang Sep 26, 2022

Choose a reason for hiding this comment

Uh oh!

Jackie-Jiang Sep 28, 2022

Choose a reason for hiding this comment

Uh oh!

KKcorps Sep 28, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Jackie-Jiang Sep 28, 2022

Choose a reason for hiding this comment

Uh oh!

KKcorps Sep 28, 2022

Choose a reason for hiding this comment

Uh oh!

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

KKcorps commented Sep 16, 2022 •

edited

Loading

codecov-commenter commented Sep 23, 2022 •

edited

Loading