Jackie-Jiang commented on code in PR #14250:
URL: https://github.com/apache/pinot/pull/14250#discussion_r1932812065
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentRestletResource.java:
##########
@@ -114,7 +114,7 @@
* <li>"/segments/{tableName}/servers": get a map from server to
segments hosted by the server</li>
* <li>"/segments/{tableName}/crc": get a map from segment to CRC of the
segment (OFFLINE table only)</li>
* <li>"/segments/{tableName}/{segmentName}/metadata: get the metadata
for a segment</li>
- * <li>"/segments/{tableName}/metadata: get the metadata for all
segments from the server</li>
+ * <li>"/v2/segments/{tableName}/metadata: get the metadata for
all/included segments from the server</li>
Review Comment:
We shouldn't need to introduce this new API. We can just add an extra query
parameter for segments
##########
pinot-controller/src/main/java/org/apache/pinot/controller/util/TableMetadataReader.java:
##########
@@ -84,11 +85,46 @@ public List<String> getReloadCheckResponses(String
tableNameWithType, int timeou
/**
* This api takes in list of segments for which we need the metadata.
+ * This calls the server to get the metadata for all segments instead of
making a call per segment.
*/
public JsonNode getSegmentsMetadata(String tableNameWithType, List<String>
columns, Set<String> segmentsToInclude,
int timeoutMs)
throws InvalidConfigException, IOException {
- return getSegmentsMetadataInternal(tableNameWithType, columns,
segmentsToInclude, timeoutMs);
+ return getSegmentsMetadataInternalV2(tableNameWithType, columns,
segmentsToInclude, timeoutMs);
+ }
+
+ private JsonNode getSegmentsMetadataInternalV2(String tableNameWithType,
List<String> columns,
Review Comment:
Let's modify the `getSegmentsMetadataInternal()` to first try the new API,
and fallback to the old way if the new API doesn't work (e.g. server still runs
old version)
##########
pinot-controller/src/main/java/org/apache/pinot/controller/util/ServerSegmentMetadataReader.java:
##########
@@ -442,6 +442,14 @@ private String generateSegmentMetadataServerURL(String
tableNameWithType, String
return String.format("%s/tables/%s/segments/%s/metadata?%s", endpoint,
tableNameWithType, segmentName, paramsStr);
}
+ public String generateTableMetadataServerURL(String tableNameWithType,
List<String> columns,
+ Set<String> segmentsToInclude,
String endpoint) {
Review Comment:
(format) Please reformat the changes
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentRestletResource.java:
##########
@@ -931,6 +933,37 @@ public String getServerMetadata(
return segmentsMetadata;
}
+ @GET
+ @Path("v2/segments/{tableName}/metadata")
+ @Authorize(targetType = TargetType.TABLE, paramName = "tableName", action =
Actions.Table.GET_METADATA)
+ @Produces(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Get the server metadata for all table segments",
+ notes = "Get the server metadata for all table segments")
+ public String getServerMetadata(
+ @ApiParam(value = "Name of the table", required = true)
@PathParam("tableName") String tableName,
+ @ApiParam(value = "OFFLINE|REALTIME") @QueryParam("type") String
tableTypeStr,
+ @ApiParam(value = "Columns name", allowMultiple = true)
@QueryParam("columns")
+ List<String> columns, @ApiParam(value = "Segments name", allowMultiple =
true) @QueryParam("segmentsToInclude")
Review Comment:
Suggest naming the query parameter `"segments"` to be more concise
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentRestletResource.java:
##########
@@ -931,6 +933,37 @@ public String getServerMetadata(
return segmentsMetadata;
}
+ @GET
+ @Path("v2/segments/{tableName}/metadata")
+ @Authorize(targetType = TargetType.TABLE, paramName = "tableName", action =
Actions.Table.GET_METADATA)
+ @Produces(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Get the server metadata for all table segments",
+ notes = "Get the server metadata for all table segments")
+ public String getServerMetadata(
+ @ApiParam(value = "Name of the table", required = true)
@PathParam("tableName") String tableName,
+ @ApiParam(value = "OFFLINE|REALTIME") @QueryParam("type") String
tableTypeStr,
+ @ApiParam(value = "Columns name", allowMultiple = true)
@QueryParam("columns")
+ List<String> columns, @ApiParam(value = "Segments name", allowMultiple =
true) @QueryParam("segmentsToInclude")
Review Comment:
Without default value, will `columns` and `segmentsToInclude` be nullable?
If so, let's annotate them as `@Nullable`
--
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-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
