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/06/30 23:35:12 UTC
[GitHub] [pinot] yuanbenson1772 opened a new pull request, #9006: Add Segment Lineage List API #9005
yuanbenson1772 opened a new pull request, #9006:
URL: https://github.com/apache/pinot/pull/9006
Currently, there is no easy way to access segment lineage entries. Adding `listSegmentLineage` Rest API on the controller for users to retrieve such IDs.
No tests added yet as core implementation of `getSegmentLineage` is already tested in `PinotHelixResourceManagerTest.java`. `PinotSegmentUploadDownloadRestletResourceTest.java` seems to test encryption functionalities only as well. Please let me know if I'm missing something or there is a proper place to add additional test that exercise this additional logic.
Manual testing
1. With valid table type combination.
<img width="889" alt="image" src="https://user-images.githubusercontent.com/25168272/176795132-ad284dff-c0b0-4d55-ab73-fe0ce7a9f7cd.png">
2. Invalid `type` parameter.
<img width="572" alt="image" src="https://user-images.githubusercontent.com/25168272/176795230-8ece4702-477f-4090-8765-369492342ff7.png">
3. Invalid table
<img width="579" alt="image" src="https://user-images.githubusercontent.com/25168272/176795304-922030a6-f2cd-4f86-bebc-7cb78911a788.png">
--
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] yuanbenson commented on pull request #9006: Add Segment Lineage List API #9005
Posted by GitBox <gi...@apache.org>.
yuanbenson commented on PR #9006:
URL: https://github.com/apache/pinot/pull/9006#issuecomment-1183491866
> (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.
--
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] yuanbenson commented on a diff in pull request #9006: Add Segment Lineage List API #9005
Posted by GitBox <gi...@apache.org>.
yuanbenson commented on code in PR #9006:
URL: https://github.com/apache/pinot/pull/9006#discussion_r920321960
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java:
##########
@@ -652,6 +652,29 @@ public Response endReplaceSegments(
}
}
+ @GET
+ @Path("segments/{tableName}/listSegmentLineage")
+ @Authenticate(AccessType.READ)
+ @Produces(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "List segment lineage", notes = "List segment lineage")
+ public Response listSegmentLineage(
Review Comment:
@walterddr Moved the newly added API to `PinotSegmentRestletResource`.
--
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] yuanbenson commented on a diff in pull request #9006: Add Segment Lineage List API #9005
Posted by GitBox <gi...@apache.org>.
yuanbenson commented on code in PR #9006:
URL: https://github.com/apache/pinot/pull/9006#discussion_r920320063
##########
pinot-common/src/main/java/org/apache/pinot/common/lineage/SegmentLineage.java:
##########
@@ -146,4 +150,19 @@ public ZNRecord toZNRecord() {
}
return znRecord;
}
+
+ /**
+ * Returns a json representation of the segment lineage.
+ * Segment lineage entries are sorted in chronological order by default.
+ */
+ public ObjectNode toJsonObject() {
+ ObjectNode jsonObject = JsonUtils.newObjectNode();
+ jsonObject.put("tableNameWithType", _tableNameWithType);
+ 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()));
Review Comment:
@snleee fyi ensures resulting entries of listing segment lineage REST API are in chronological order 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
[GitHub] [pinot] walterddr commented on a diff in pull request #9006: Add Segment Lineage List API #9005
Posted by GitBox <gi...@apache.org>.
walterddr commented on code in PR #9006:
URL: https://github.com/apache/pinot/pull/9006#discussion_r920336334
##########
pinot-common/src/main/java/org/apache/pinot/common/lineage/SegmentLineage.java:
##########
@@ -146,4 +150,19 @@ public ZNRecord toZNRecord() {
}
return znRecord;
}
+
+ /**
+ * Returns a json representation of the segment lineage.
+ * Segment lineage entries are sorted in chronological order by default.
+ */
+ public ObjectNode toJsonObject() {
+ ObjectNode jsonObject = JsonUtils.newObjectNode();
+ jsonObject.put("tableNameWithType", _tableNameWithType);
+ 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()));
Review Comment:
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?
--
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 #9006: Add Segment Lineage List API #9005
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #9006:
URL: https://github.com/apache/pinot/pull/9006#issuecomment-1175603639
# [Codecov](https://codecov.io/gh/apache/pinot/pull/9006?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 [#9006](https://codecov.io/gh/apache/pinot/pull/9006?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ddb5c68) into [master](https://codecov.io/gh/apache/pinot/commit/378bdec11c68a54366cd98b0f2eda5807e454e2f?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (378bdec) will **decrease** coverage by `53.31%`.
> The diff coverage is `28.02%`.
> :exclamation: Current head ddb5c68 differs from pull request most recent head 4d474cd. Consider uploading reports for the commit 4d474cd to get more accurate results
```diff
@@ Coverage Diff @@
## master #9006 +/- ##
=============================================
- Coverage 68.55% 15.24% -53.32%
+ Complexity 4839 170 -4669
=============================================
Files 1808 1777 -31
Lines 94274 93824 -450
Branches 14056 14110 +54
=============================================
- Hits 64633 14306 -50327
- Misses 25122 78495 +53373
+ Partials 4519 1023 -3496
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| unittests1 | `?` | |
| unittests2 | `15.24% <28.02%> (+0.36%)` | :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/9006?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...roker/requesthandler/BaseBrokerRequestHandler.java](https://codecov.io/gh/apache/pinot/pull/9006/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-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcmVxdWVzdGhhbmRsZXIvQmFzZUJyb2tlclJlcXVlc3RIYW5kbGVyLmphdmE=) | `32.99% <0.00%> (-35.47%)` | :arrow_down: |
| [...roker/requesthandler/GrpcBrokerRequestHandler.java](https://codecov.io/gh/apache/pinot/pull/9006/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-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcmVxdWVzdGhhbmRsZXIvR3JwY0Jyb2tlclJlcXVlc3RIYW5kbGVyLmphdmE=) | `0.00% <0.00%> (ø)` | |
| [.../pinot/common/function/scalar/DateTimeConvert.java](https://codecov.io/gh/apache/pinot/pull/9006/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vc2NhbGFyL0RhdGVUaW1lQ29udmVydC5qYXZh) | `0.00% <0.00%> (ø)` | |
| [...inot/common/function/scalar/DateTimeFunctions.java](https://codecov.io/gh/apache/pinot/pull/9006/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vc2NhbGFyL0RhdGVUaW1lRnVuY3Rpb25zLmphdmE=) | `0.00% <ø> (-95.24%)` | :arrow_down: |
| [...ache/pinot/common/metadata/ZKMetadataProvider.java](https://codecov.io/gh/apache/pinot/pull/9006/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0YWRhdGEvWktNZXRhZGF0YVByb3ZpZGVyLmphdmE=) | `0.00% <0.00%> (-64.81%)` | :arrow_down: |
| [...a/org/apache/pinot/common/metrics/BrokerTimer.java](https://codecov.io/gh/apache/pinot/pull/9006/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9Ccm9rZXJUaW1lci5qYXZh) | `0.00% <0.00%> (-94.74%)` | :arrow_down: |
| [...g/apache/pinot/common/metrics/ControllerGauge.java](https://codecov.io/gh/apache/pinot/pull/9006/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9Db250cm9sbGVyR2F1Z2UuamF2YQ==) | `0.00% <0.00%> (-97.68%)` | :arrow_down: |
| [...a/org/apache/pinot/common/metrics/ServerMeter.java](https://codecov.io/gh/apache/pinot/pull/9006/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9TZXJ2ZXJNZXRlci5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...g/apache/pinot/common/minion/BaseTaskMetadata.java](https://codecov.io/gh/apache/pinot/pull/9006/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWluaW9uL0Jhc2VUYXNrTWV0YWRhdGEuamF2YQ==) | `0.00% <ø> (-60.00%)` | :arrow_down: |
| [...e/pinot/common/minion/MergeRollupTaskMetadata.java](https://codecov.io/gh/apache/pinot/pull/9006/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWluaW9uL01lcmdlUm9sbHVwVGFza01ldGFkYXRhLmphdmE=) | `0.00% <ø> (-94.74%)` | :arrow_down: |
| ... and [1555 more](https://codecov.io/gh/apache/pinot/pull/9006/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) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/9006?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/9006?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [378bdec...4d474cd](https://codecov.io/gh/apache/pinot/pull/9006?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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] snleee commented on pull request #9006: Add Segment Lineage List API #9005
Posted by GitBox <gi...@apache.org>.
snleee commented on PR #9006:
URL: https://github.com/apache/pinot/pull/9006#issuecomment-1180939560
@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?
--
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] snleee merged pull request #9006: Add Segment Lineage List API #9005
Posted by GitBox <gi...@apache.org>.
snleee merged PR #9006:
URL: https://github.com/apache/pinot/pull/9006
--
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] jtao15 commented on pull request #9006: Add Segment Lineage List API #9005
Posted by GitBox <gi...@apache.org>.
jtao15 commented on PR #9006:
URL: https://github.com/apache/pinot/pull/9006#issuecomment-1171778972
Looks like it only returns the lineage entry Ids, can we return the actual entries as well?
--
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] sajjad-moradi commented on a diff in pull request #9006: Add Segment Lineage List API #9005
Posted by GitBox <gi...@apache.org>.
sajjad-moradi commented on code in PR #9006:
URL: https://github.com/apache/pinot/pull/9006#discussion_r911522746
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java:
##########
@@ -652,6 +652,29 @@ public Response endReplaceSegments(
}
}
+ @GET
+ @Path("segments/{tableName}/listSegmentLineage")
+ @Authenticate(AccessType.READ)
+ @Produces(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "List segment lineage", notes = "List segment lineage")
+ public Response listSegmentLineage(
+ @ApiParam(value = "Name of the table", required = true) @PathParam("tableName") String tableName,
+ @ApiParam(value = "OFFLINE|REALTIME", required = true) @QueryParam("type") String tableTypeStr) {
+ TableType tableType = Constants.validateTableType(tableTypeStr);
+ if (tableType == null) {
+ throw new ControllerApplicationException(LOGGER, "Table type should either be offline or realtime",
+ Response.Status.BAD_REQUEST);
+ }
+ try {
+ String tableNameWithType =
+ ResourceUtils.getExistingTableNamesWithType(_pinotHelixResourceManager, tableName, tableType, LOGGER).get(0);
Review Comment:
I think this already throws 404 if the table is not there. You can move it outside the try-catch and change the error in the catch block for generic exception to 500.
--
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] snleee commented on a diff in pull request #9006: Add Segment Lineage List API #9005
Posted by GitBox <gi...@apache.org>.
snleee commented on code in PR #9006:
URL: https://github.com/apache/pinot/pull/9006#discussion_r920361594
##########
pinot-common/src/main/java/org/apache/pinot/common/lineage/SegmentLineage.java:
##########
@@ -146,4 +150,19 @@ public ZNRecord toZNRecord() {
}
return znRecord;
}
+
+ /**
+ * Returns a json representation of the segment lineage.
+ * Segment lineage entries are sorted in chronological order by default.
+ */
+ public ObjectNode toJsonObject() {
+ ObjectNode jsonObject = JsonUtils.newObjectNode();
+ jsonObject.put("tableNameWithType", _tableNameWithType);
+ 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()));
Review Comment:
discussed offline. We agreed to file the issue and address it from the separate 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] jtao15 commented on pull request #9006: Add Segment Lineage List API #9005
Posted by GitBox <gi...@apache.org>.
jtao15 commented on PR #9006:
URL: https://github.com/apache/pinot/pull/9006#issuecomment-1183444137
LGTM, thanks for working on this!
--
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] snleee commented on pull request #9006: Add Segment Lineage List API #9005
Posted by GitBox <gi...@apache.org>.
snleee commented on PR #9006:
URL: https://github.com/apache/pinot/pull/9006#issuecomment-1183489090
@yuanbenson Can you change the description `segments/{tableName}/listSegmentLineage`?
--
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] yuanbenson1772 commented on a diff in pull request #9006: Add Segment Lineage List API #9005
Posted by GitBox <gi...@apache.org>.
yuanbenson1772 commented on code in PR #9006:
URL: https://github.com/apache/pinot/pull/9006#discussion_r917465894
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java:
##########
@@ -652,6 +652,29 @@ public Response endReplaceSegments(
}
}
+ @GET
+ @Path("segments/{tableName}/listSegmentLineage")
+ @Authenticate(AccessType.READ)
+ @Produces(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "List segment lineage", notes = "List segment lineage")
+ public Response listSegmentLineage(
Review Comment:
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.
--
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] yuanbenson1772 commented on pull request #9006: Add Segment Lineage List API #9005
Posted by GitBox <gi...@apache.org>.
yuanbenson1772 commented on PR #9006:
URL: https://github.com/apache/pinot/pull/9006#issuecomment-1179827469
> 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.
--
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] walterddr commented on a diff in pull request #9006: Add Segment Lineage List API #9005
Posted by GitBox <gi...@apache.org>.
walterddr commented on code in PR #9006:
URL: https://github.com/apache/pinot/pull/9006#discussion_r918044682
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java:
##########
@@ -652,6 +652,29 @@ public Response endReplaceSegments(
}
}
+ @GET
+ @Path("segments/{tableName}/listSegmentLineage")
+ @Authenticate(AccessType.READ)
+ @Produces(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "List segment lineage", notes = "List segment lineage")
+ public Response listSegmentLineage(
Review Comment:
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`
--
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] walterddr commented on a diff in pull request #9006: Add Segment Lineage List API #9005
Posted by GitBox <gi...@apache.org>.
walterddr commented on code in PR #9006:
URL: https://github.com/apache/pinot/pull/9006#discussion_r918044682
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java:
##########
@@ -652,6 +652,29 @@ public Response endReplaceSegments(
}
}
+ @GET
+ @Path("segments/{tableName}/listSegmentLineage")
+ @Authenticate(AccessType.READ)
+ @Produces(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "List segment lineage", notes = "List segment lineage")
+ public Response listSegmentLineage(
Review Comment:
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 APIs are located.
then it should be easy to add a test in `PinotSegmentRestletResourceTest`
--
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] walterddr commented on a diff in pull request #9006: Add Segment Lineage List API #9005
Posted by GitBox <gi...@apache.org>.
walterddr commented on code in PR #9006:
URL: https://github.com/apache/pinot/pull/9006#discussion_r918399759
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java:
##########
@@ -652,6 +652,29 @@ public Response endReplaceSegments(
}
}
+ @GET
+ @Path("segments/{tableName}/listSegmentLineage")
+ @Authenticate(AccessType.READ)
+ @Produces(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "List segment lineage", notes = "List segment lineage")
+ public Response listSegmentLineage(
Review Comment:
CC ^ @snleee what do you think? this IMO should be in SegmentRestletResource rather than the UploadDownload one
--
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] walterddr commented on a diff in pull request #9006: Add Segment Lineage List API #9005
Posted by GitBox <gi...@apache.org>.
walterddr commented on code in PR #9006:
URL: https://github.com/apache/pinot/pull/9006#discussion_r914273106
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java:
##########
@@ -652,6 +652,29 @@ public Response endReplaceSegments(
}
}
+ @GET
+ @Path("segments/{tableName}/listSegmentLineage")
+ @Authenticate(AccessType.READ)
+ @Produces(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "List segment lineage", notes = "List segment lineage")
+ public Response listSegmentLineage(
Review Comment:
could we add some test for this endpoint. you can checkout how other endpoints are tested under `PinotSegmentUploadDownloadRestletResourceTest`
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java:
##########
@@ -3274,6 +3274,15 @@ public void endReplaceSegments(String tableNameWithType, String segmentLineageEn
tableNameWithType, segmentLineageEntryId);
}
+ /**
+ * List the segment lineage
+ *
+ * @param tableNameWithType
+ */
+ public SegmentLineage listSegmentLineage(String tableNameWithType) {
+ return SegmentLineageAccessHelper.getSegmentLineage(_propertyStore, tableNameWithType);
+ }
Review Comment:
i think @jtao15 already mentioned. it is better to return SegmentLineage. you are already doing it here. is it possible to you to update the PR description to show the accurate response screenshot. thanks.
--
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] walterddr commented on a diff in pull request #9006: Add Segment Lineage List API #9005
Posted by GitBox <gi...@apache.org>.
walterddr commented on code in PR #9006:
URL: https://github.com/apache/pinot/pull/9006#discussion_r920336334
##########
pinot-common/src/main/java/org/apache/pinot/common/lineage/SegmentLineage.java:
##########
@@ -146,4 +150,19 @@ public ZNRecord toZNRecord() {
}
return znRecord;
}
+
+ /**
+ * Returns a json representation of the segment lineage.
+ * Segment lineage entries are sorted in chronological order by default.
+ */
+ public ObjectNode toJsonObject() {
+ ObjectNode jsonObject = JsonUtils.newObjectNode();
+ jsonObject.put("tableNameWithType", _tableNameWithType);
+ 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()));
Review Comment:
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 when we support filtering arguments, for example: `segments/{tableName}/lineage?timestamp>-10days`
--
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] jtao15 commented on pull request #9006: Add Segment Lineage List API #9005
Posted by GitBox <gi...@apache.org>.
jtao15 commented on PR #9006:
URL: https://github.com/apache/pinot/pull/9006#issuecomment-1183491128
(minor) Can we change `segments/{tableName}/listSegmentLineage` to `segments/{tableName}/lineage` to make this simpler?
--
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] snleee commented on a diff in pull request #9006: Add Segment Lineage List API #9005
Posted by GitBox <gi...@apache.org>.
snleee commented on code in PR #9006:
URL: https://github.com/apache/pinot/pull/9006#discussion_r919183805
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/builder/ControllerRequestURLBuilder.java:
##########
@@ -344,6 +344,10 @@ public String forSegmentMetadata(String tableName, String segmentName) {
return StringUtil.join("/", _baseUrl, "segments", tableName, encode(segmentName), "metadata");
}
+ public String forSegmentLineage(String tableName, String tableType) {
Review Comment:
`forListAllSegmentLineages` would be better naming (following the convention with other functions)
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java:
##########
@@ -652,6 +652,29 @@ public Response endReplaceSegments(
}
}
+ @GET
+ @Path("segments/{tableName}/listSegmentLineage")
+ @Authenticate(AccessType.READ)
+ @Produces(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "List segment lineage", notes = "List segment lineage")
+ public Response listSegmentLineage(
Review Comment:
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.
--
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] snleee commented on pull request #9006: Add Segment Lineage List API #9005
Posted by GitBox <gi...@apache.org>.
snleee commented on PR #9006:
URL: https://github.com/apache/pinot/pull/9006#issuecomment-1183519825
@yuanbenson Please create an issue regarding optimizing the JSON conversion for the `SegmentLineage` to remove the necessity of sorting.
--
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