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 2021/12/07 22:30:46 UTC
[GitHub] [pinot] jackjlli opened a new pull request #7878: Add parameter to list segments which can be queried
jackjlli opened a new pull request #7878:
URL: https://github.com/apache/pinot/pull/7878
## Description
This PR adds a parameter called `liveOnly` to listSegments API in order to return live only segments which can be queried.
This is useful for us to know which set of segments can be queried.
One example is that if merged segments are generated by merge & roll up feature, the old segments won't be queried any more. In this case, user may not be interested in knowing the old segments.
## Upgrade Notes
Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)
* [ ] Yes (Please label as **<code>backward-incompat</code>**, and complete the section below on Release Notes)
Does this PR fix a zero-downtime upgrade introduced earlier?
* [ ] Yes (Please label this as **<code>backward-incompat</code>**, and complete the section below on Release Notes)
Does this PR otherwise need attention when creating release notes? Things to consider:
- New configuration options
- Deprecation of configurations
- Signature changes to public methods/interfaces
- New plugins added or old plugins removed
* [ ] Yes (Please label this PR as **<code>release-notes</code>** and complete the section on Release Notes)
## Release Notes
<!-- If you have tagged this as either backward-incompat or release-notes,
you MUST add text here that you would like to see appear in release notes of the
next release. -->
<!-- If you have a series of commits adding or enabling a feature, then
add this section only in final commit that marks the feature completed.
Refer to earlier release notes to see examples of text.
-->
## Documentation
<!-- If you have introduced a new feature or configuration, please add it to the documentation as well.
See https://docs.pinot.apache.org/developers/developers-and-contributors/update-document
-->
--
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] jackjlli commented on a change in pull request #7878: Add parameter to list segments which can be queried
Posted by GitBox <gi...@apache.org>.
jackjlli commented on a change in pull request #7878:
URL: https://github.com/apache/pinot/pull/7878#discussion_r766229138
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentRestletResource.java
##########
@@ -163,17 +163,21 @@
@GET
@Produces(MediaType.APPLICATION_JSON)
@Path("/segments/{tableName}")
- @ApiOperation(value = "List all segments", notes = "List all segments")
+ @ApiOperation(value = "List all segments. The 'liveOnly' parameter is used to get the list of segments"
+ + " which can be queried from the table. The value is false by default.", notes = "List all segments")
public List<Map<TableType, List<String>>> getSegments(
@ApiParam(value = "Name of the table", required = true) @PathParam("tableName") String tableName,
- @ApiParam(value = "OFFLINE|REALTIME") @QueryParam("type") String tableTypeStr) {
+ @ApiParam(value = "OFFLINE|REALTIME") @QueryParam("type") String tableTypeStr,
+ @ApiParam(value = "Whether to return live only segments, which is a set of segments that can be queried"
Review comment:
That's a good point! I'll polish the description to make it clear in the next push. In the future, if we really want to exclude the segment in ERROR state, we can also bring up another param like `excludeErrorSegments` and the like.
--
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] Jackie-Jiang commented on a change in pull request #7878: Add parameter to list segments which can be queried
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #7878:
URL: https://github.com/apache/pinot/pull/7878#discussion_r765329927
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentRestletResource.java
##########
@@ -163,17 +163,21 @@
@GET
@Produces(MediaType.APPLICATION_JSON)
@Path("/segments/{tableName}")
- @ApiOperation(value = "List all segments", notes = "List all segments")
+ @ApiOperation(value = "List all segments. The 'liveOnly' parameter is used to get the list of segments"
+ + " which can be queried from the table. The value is false by default.", notes = "List all segments")
public List<Map<TableType, List<String>>> getSegments(
@ApiParam(value = "Name of the table", required = true) @PathParam("tableName") String tableName,
- @ApiParam(value = "OFFLINE|REALTIME") @QueryParam("type") String tableTypeStr) {
+ @ApiParam(value = "OFFLINE|REALTIME") @QueryParam("type") String tableTypeStr,
+ @ApiParam(value = "Whether to return live only segments, which is a set of segments that can be queried"
Review comment:
This is a little bit vague to me. Here essentially we are excluding the segments already replaced using the lineage info, and not excluding the segments that are either in ERROR state or not served by the servers. Suggest updating the description/method name to make that clear
--
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 change in pull request #7878: Add parameter to list segments which can be queried
Posted by GitBox <gi...@apache.org>.
snleee commented on a change in pull request #7878:
URL: https://github.com/apache/pinot/pull/7878#discussion_r768162887
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentRestletResource.java
##########
@@ -163,17 +163,21 @@
@GET
@Produces(MediaType.APPLICATION_JSON)
@Path("/segments/{tableName}")
- @ApiOperation(value = "List all segments", notes = "List all segments")
+ @ApiOperation(value = "List all segments. The 'liveOnly' parameter is used to get the list of segments"
+ + " which can be queried from the table. The value is false by default.", notes = "List all segments")
public List<Map<TableType, List<String>>> getSegments(
@ApiParam(value = "Name of the table", required = true) @PathParam("tableName") String tableName,
- @ApiParam(value = "OFFLINE|REALTIME") @QueryParam("type") String tableTypeStr) {
+ @ApiParam(value = "OFFLINE|REALTIME") @QueryParam("type") String tableTypeStr,
+ @ApiParam(value = "Whether to return live only segments, which is a set of segments that can be queried"
Review comment:
Can you change the description so that this can be in sync with the new parameter name (`excludeReplacedSegments`)?
--
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] jackjlli commented on a change in pull request #7878: Add parameter to list segments which can be queried
Posted by GitBox <gi...@apache.org>.
jackjlli commented on a change in pull request #7878:
URL: https://github.com/apache/pinot/pull/7878#discussion_r766228422
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentRestletResource.java
##########
@@ -163,17 +163,21 @@
@GET
@Produces(MediaType.APPLICATION_JSON)
@Path("/segments/{tableName}")
- @ApiOperation(value = "List all segments", notes = "List all segments")
+ @ApiOperation(value = "List all segments. The 'liveOnly' parameter is used to get the list of segments"
Review comment:
There can be multiple kinds of segments that should be ignored. E.g. like Jackie mentioned, segments in ERROR state or not served by servers can also be ignored. Plus, the existing behavior should the default behavior, which means that even if we introduce this `includeIgnoredSegments`, the value should be false by default.
How about renaming it to `queryableSegments` and make it false by default?
--
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] jackjlli merged pull request #7878: Add parameter to list segments which can be queried
Posted by GitBox <gi...@apache.org>.
jackjlli merged pull request #7878:
URL: https://github.com/apache/pinot/pull/7878
--
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] jackjlli commented on a change in pull request #7878: Add parameter to list segments which can be queried
Posted by GitBox <gi...@apache.org>.
jackjlli commented on a change in pull request #7878:
URL: https://github.com/apache/pinot/pull/7878#discussion_r764452620
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentRestletResource.java
##########
@@ -166,14 +166,16 @@
@ApiOperation(value = "List all segments", notes = "List all segments")
public List<Map<TableType, List<String>>> getSegments(
@ApiParam(value = "Name of the table", required = true) @PathParam("tableName") String tableName,
- @ApiParam(value = "OFFLINE|REALTIME") @QueryParam("type") String tableTypeStr) {
+ @ApiParam(value = "OFFLINE|REALTIME") @QueryParam("type") String tableTypeStr,
+ @ApiParam(value = "Whether live only segments will be returned") @QueryParam("liveOnly") String liveOnly) {
Review comment:
Added more description to the API.
--
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] mcvsubbu commented on a change in pull request #7878: Add parameter to list segments which can be queried
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #7878:
URL: https://github.com/apache/pinot/pull/7878#discussion_r766154068
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentRestletResource.java
##########
@@ -163,17 +163,21 @@
@GET
@Produces(MediaType.APPLICATION_JSON)
@Path("/segments/{tableName}")
- @ApiOperation(value = "List all segments", notes = "List all segments")
+ @ApiOperation(value = "List all segments. The 'liveOnly' parameter is used to get the list of segments"
Review comment:
All the internal implementation can be fixed, let us make sure we get the parameter name right. Just suggesting here: How about "includeIgnoredSegments"? You can write an explanation that segments are ignored when the lineage information indicates that they are superseded by newer segments.
--
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 change in pull request #7878: Add parameter to list segments which can be queried
Posted by GitBox <gi...@apache.org>.
snleee commented on a change in pull request #7878:
URL: https://github.com/apache/pinot/pull/7878#discussion_r764411941
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentRestletResource.java
##########
@@ -166,14 +166,16 @@
@ApiOperation(value = "List all segments", notes = "List all segments")
public List<Map<TableType, List<String>>> getSegments(
@ApiParam(value = "Name of the table", required = true) @PathParam("tableName") String tableName,
- @ApiParam(value = "OFFLINE|REALTIME") @QueryParam("type") String tableTypeStr) {
+ @ApiParam(value = "OFFLINE|REALTIME") @QueryParam("type") String tableTypeStr,
+ @ApiParam(value = "Whether live only segments will be returned") @QueryParam("liveOnly") String liveOnly) {
Review comment:
I think that we need to add more explanation on what does it mean by `live segments`.
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
##########
@@ -617,12 +622,21 @@ public String getCrypterClassNameFromTableConfig(String tableNameWithType) {
}
}
}
+ return retainLiveSegments(tableNameWithType, selectedSegments);
+ }
+
+ /**
+ * Given the list of segment names, retains live segments which are queried.
+ * @param tableNameWithType table name with type
+ * @param segments list of input segment names
+ */
+ private List<String> retainLiveSegments(String tableNameWithType, List<String> segments) {
Review comment:
`computeLiveSegments`?
--
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] jackjlli commented on a change in pull request #7878: Add parameter to list segments which can be queried
Posted by GitBox <gi...@apache.org>.
jackjlli commented on a change in pull request #7878:
URL: https://github.com/apache/pinot/pull/7878#discussion_r768173218
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentRestletResource.java
##########
@@ -163,17 +163,21 @@
@GET
@Produces(MediaType.APPLICATION_JSON)
@Path("/segments/{tableName}")
- @ApiOperation(value = "List all segments", notes = "List all segments")
+ @ApiOperation(value = "List all segments. The 'liveOnly' parameter is used to get the list of segments"
+ + " which can be queried from the table. The value is false by default.", notes = "List all segments")
public List<Map<TableType, List<String>>> getSegments(
@ApiParam(value = "Name of the table", required = true) @PathParam("tableName") String tableName,
- @ApiParam(value = "OFFLINE|REALTIME") @QueryParam("type") String tableTypeStr) {
+ @ApiParam(value = "OFFLINE|REALTIME") @QueryParam("type") String tableTypeStr,
+ @ApiParam(value = "Whether to return live only segments, which is a set of segments that can be queried"
Review comment:
Description updated.
--
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 #7878: Add parameter to list segments which can be queried
Posted by GitBox <gi...@apache.org>.
jtao15 commented on pull request #7878:
URL: https://github.com/apache/pinot/pull/7878#issuecomment-988367428
Thanks for the pr, LGTM
--
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] jackjlli commented on a change in pull request #7878: Add parameter to list segments which can be queried
Posted by GitBox <gi...@apache.org>.
jackjlli commented on a change in pull request #7878:
URL: https://github.com/apache/pinot/pull/7878#discussion_r764452526
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
##########
@@ -617,12 +622,21 @@ public String getCrypterClassNameFromTableConfig(String tableNameWithType) {
}
}
}
+ return retainLiveSegments(tableNameWithType, selectedSegments);
+ }
+
+ /**
+ * Given the list of segment names, retains live segments which are queried.
+ * @param tableNameWithType table name with type
+ * @param segments list of input segment names
+ */
+ private List<String> retainLiveSegments(String tableNameWithType, List<String> segments) {
Review comment:
Renamed.
--
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