Add parameter to list segments which can be queried #7878
Conversation
...ler/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentRestletResource.java
Outdated
Show resolved
Hide resolved
...ntroller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
Outdated
Show resolved
Hide resolved
jackjlli
force-pushed
the
return-live-segments
branch
from
413c9df to
f42eac8
Compare
|
Thanks for the pr, LGTM |
| 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" |
There was a problem hiding this 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
There was a problem hiding this 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.
There was a problem hiding this comment.
Can you change the description so that this can be in sync with the new parameter name (excludeReplacedSegments)?
| @@ -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" | |||
There was a problem hiding this 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.
There was a problem hiding this 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?
There was a problem hiding this comment.
Discussed it offline. The existing listSegments returns all the segments regardless of their states. Thus, the optional parameter should be called sth like excludeReplacedSegments to indicate that replaced segments are filtered out in the response payload. Later on, we can add more and more parameters like excludeErrorSegments, excludeConsumingSegments to filter more segments.
jackjlli
force-pushed
the
return-live-segments
branch
3 times, most recently
from
b8795b4 to
d9054f2
Compare
jackjlli
force-pushed
the
return-live-segments
branch
from
d9054f2 to
6765116
Compare
Description
This PR adds a parameter called
excludeReplacedSegmentsto 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)
backward-incompat, and complete the section below on Release Notes)Does this PR fix a zero-downtime upgrade introduced earlier?
backward-incompat, and complete the section below on Release Notes)Does this PR otherwise need attention when creating release notes? Things to consider:
release-notesand complete the section on Release Notes)Release Notes
Documentation