Add Segment Lineage List API #9005 #9006

yuanbenson · 2022-06-30T23:35:09Z

Description

This PR adds support for listing segment lineage entries. The following endpoint on the controller can be used to retrieve such entries in chronological order of timestamp for a particular table.

segments/{tableName}/lineage

Testing Done

Added new Junit test in PinotSegmentRestletResourceTest.java

Manual verification

With valid table type combination.

Invalid type parameter.

3. Invalid table

Labels

feature

jtao15 · 2022-06-30T23:52:51Z

Looks like it only returns the lineage entry Ids, can we return the actual entries as well? You can probably refer the get schema api "/schema/{schemaName}" to convert the object to json string.

...ava/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java

walterddr · 2022-07-05T23:07:36Z

...ava/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java

+  @Authenticate(AccessType.READ)
+  @Produces(MediaType.APPLICATION_JSON)
+  @ApiOperation(value = "List segment lineage", notes = "List segment lineage")
+  public Response listSegmentLineage(


could we add some test for this endpoint. you can checkout how other endpoints are tested under PinotSegmentUploadDownloadRestletResourceTest

Raised some concerns in the #Testing done section of the PR description. Namely, PinotSegmentUploadDownloadRestletResourceTest does not seem to cover any segment operations integration tests (as it requires table creation and segment upload) and such operations are tested in PinotHelixResourceManagerTest though.

oh. actually my mistake. I think the first thing I suggest is to move the API from PinotSegmentUploadDownloadRestletResource to PinotSegmentRestletResource where all the other segment metadata APIs are located.
then it should be easy to add a test in PinotSegmentRestletResourceTest

CC ^ @snleee what do you think? this IMO should be in SegmentRestletResource rather than the UploadDownload one

I think that @yuanbenson probably put the new API to PinotSegmentUploadDownloadRestletResource because the segment lineage-related code is being written in the segment replacement-related APIs.

Another way to put this is e.g. GET /tables/{tableName}/segments/lineage under PinotSegmentRestletResource as @walterddr suggested.

@walterddr Moved the newly added API to PinotSegmentRestletResource.

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

codecov-commenter · 2022-07-05T23:38:12Z

Codecov Report

Merging #9006 (a018c41) into master (378bdec) will decrease coverage by 0.08%.
The diff coverage is 73.35%.

❗ Current head a018c41 differs from pull request most recent head 3be8b5e. Consider uploading reports for the commit 3be8b5e to get more accurate results

@@             Coverage Diff              @@
##             master    #9006      +/-   ##
============================================
- Coverage     68.55%   68.47%   -0.09%     
- Complexity     4839     4961     +122     
============================================
  Files          1808     1831      +23     
  Lines         94274    96293    +2019     
  Branches      14056    14392     +336     
============================================
+ Hits          64633    65935    +1302     
- Misses        25122    25774     +652     
- Partials       4519     4584      +65

Flag	Coverage Δ
integration1	`?`
integration2	`24.64% <29.52%> (?)`
unittests1	`66.85% <68.46%> (+0.42%)`	⬆️
unittests2	`15.40% <28.01%> (+0.52%)`	⬆️

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

Impacted Files	Coverage Δ
.../java/org/apache/pinot/client/utils/Constants.java	`0.00% <ø> (ø)`
...ava/org/apache/pinot/client/utils/DriverUtils.java	`15.73% <ø> (+11.33%)`	⬆️
...inot/common/function/scalar/DateTimeFunctions.java	`95.00% <ø> (-0.24%)`	⬇️
...rg/apache/pinot/common/lineage/SegmentLineage.java	`72.22% <0.00%> (-23.43%)`	⬇️
...g/apache/pinot/common/minion/BaseTaskMetadata.java	`60.00% <ø> (ø)`
...e/pinot/common/minion/MergeRollupTaskMetadata.java	`0.00% <ø> (-94.74%)`	⬇️
.../minion/RealtimeToOfflineSegmentsTaskMetadata.java	`100.00% <ø> (ø)`
...pache/pinot/common/utils/request/RequestUtils.java	`82.89% <0.00%> (+1.31%)`	⬆️
...roller/api/resources/PinotTaskRestletResource.java	`0.00% <0.00%> (-4.06%)`	⬇️
...troller/helix/core/minion/ClusterInfoAccessor.java	`65.51% <ø> (-13.80%)`	⬇️
... and 521 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 378bdec...3be8b5e. Read the comment docs.

yuanbenson · 2022-07-10T23:54:40Z

Looks like it only returns the lineage entry Ids, can we return the actual entries as well? You can probably refer the get schema api "/schema/{schemaName}" to convert the object to json string.

Thanks, addressed.

snleee

LGTM. Thank you for working on this!

snleee · 2022-07-11T22:27:43Z

@yuanbenson Linter test fails. Can you try to set up https://docs.pinot.apache.org/developers/developers-and-contributors/code-setup#intellij and apply the style to the classes that you added?

pinot-spi/src/main/java/org/apache/pinot/spi/utils/builder/ControllerRequestURLBuilder.java

snleee · 2022-07-12T16:42:23Z

...ava/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java

+  @Authenticate(AccessType.READ)
+  @Produces(MediaType.APPLICATION_JSON)
+  @ApiOperation(value = "List segment lineage", notes = "List segment lineage")
+  public Response listSegmentLineage(


I think that @yuanbenson probably put the new API to PinotSegmentUploadDownloadRestletResource because the segment lineage-related code is being written in the segment replacement-related APIs.

Another way to put this is e.g. GET /tables/{tableName}/segments/lineage under PinotSegmentRestletResource as @walterddr suggested.

jtao15 · 2022-07-13T16:39:08Z

LGTM, thanks for working on this!

yuanbenson · 2022-07-13T17:10:50Z

pinot-common/src/main/java/org/apache/pinot/common/lineage/SegmentLineage.java

+    LinkedHashMap<String, LineageEntry> sortedLineageEntries = new LinkedHashMap<>();
+    _lineageEntries.entrySet().stream()
+        .sorted(Map.Entry.comparingByValue(Comparator.comparingLong(LineageEntry::getTimestamp)))
+        .forEachOrdered(x -> sortedLineageEntries.put(x.getKey(), x.getValue()));


@snleee fyi ensures resulting entries of listing segment lineage REST API are in chronological order here.

IMO sorting is not necessary. at least we shouldn't do this in the toJsonObject path. what do you think we add the timestamp processing later?

discussed offline. We agreed to file the issue and address it from the separate PR.

snleee · 2022-07-13T17:25:09Z

@yuanbenson Can you change the description segments/{tableName}/listSegmentLineage?

yuanbenson · 2022-07-13T17:28:14Z

(minor) Can we change segments/{tableName}/listSegmentLineage to segments/{tableName}/lineage to make this simpler?

Yes, indeed that has been done! I just updated the description to reflect that.

snleee · 2022-07-13T17:59:54Z

@yuanbenson Please create an issue regarding optimizing the JSON conversion for the SegmentLineage to remove the necessity of sorting.

walterddr

looks good to me. thank you for the contribution!

yuanbenson marked this pull request as ready for review June 30, 2022 23:35

sajjad-moradi reviewed Jul 1, 2022

View reviewed changes

...ava/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java Outdated Show resolved Hide resolved

walterddr reviewed Jul 5, 2022

View reviewed changes

snleee approved these changes Jul 11, 2022

View reviewed changes

yuanbenson force-pushed the master branch from 68ab89c to 2398cb0 Compare July 12, 2022 02:37

snleee reviewed Jul 12, 2022

View reviewed changes

yuanbenson force-pushed the master branch from 2398cb0 to 6a86ecb Compare July 12, 2022 17:28

yuanbenson force-pushed the master branch from 6a86ecb to 8620298 Compare July 13, 2022 17:00

Add Segment Lineage List API apache#9005

3be8b5e

yuanbenson force-pushed the master branch from 8620298 to 3be8b5e Compare July 13, 2022 17:06

yuanbenson commented Jul 13, 2022

View reviewed changes

yuanbenson requested review from walterddr, snleee and sajjad-moradi July 13, 2022 17:13

walterddr approved these changes Jul 13, 2022

View reviewed changes

snleee merged commit 7eec296 into apache:master Jul 13, 2022

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Segment Lineage List API #9005 #9006

Add Segment Lineage List API #9005 #9006

yuanbenson commented Jun 30, 2022 •

edited

Loading

jtao15 commented Jun 30, 2022 •

edited

Loading

walterddr Jul 5, 2022

yuanbenson Jul 11, 2022

walterddr Jul 11, 2022 •

edited

Loading

walterddr Jul 11, 2022

snleee Jul 12, 2022

yuanbenson Jul 13, 2022

codecov-commenter commented Jul 5, 2022 •

edited

Loading

yuanbenson commented Jul 10, 2022

snleee left a comment

snleee commented Jul 11, 2022

snleee Jul 12, 2022

jtao15 commented Jul 13, 2022

yuanbenson Jul 13, 2022

walterddr Jul 13, 2022 •

edited

Loading

snleee Jul 13, 2022

snleee commented Jul 13, 2022

yuanbenson commented Jul 13, 2022

snleee commented Jul 13, 2022

walterddr left a comment

Add Segment Lineage List API #9005 #9006

Add Segment Lineage List API #9005 #9006

Conversation

yuanbenson commented Jun 30, 2022 • edited Loading

Description

Testing Done

Labels

jtao15 commented Jun 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

walterddr Jul 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jul 5, 2022 • edited Loading

Codecov Report

yuanbenson commented Jul 10, 2022

snleee left a comment

Choose a reason for hiding this comment

snleee commented Jul 11, 2022

Choose a reason for hiding this comment

jtao15 commented Jul 13, 2022

Choose a reason for hiding this comment

walterddr Jul 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snleee commented Jul 13, 2022

yuanbenson commented Jul 13, 2022

snleee commented Jul 13, 2022

walterddr left a comment

Choose a reason for hiding this comment

yuanbenson commented Jun 30, 2022 •

edited

Loading

jtao15 commented Jun 30, 2022 •

edited

Loading

walterddr Jul 11, 2022 •

edited

Loading

codecov-commenter commented Jul 5, 2022 •

edited

Loading

walterddr Jul 13, 2022 •

edited

Loading