somandal commented on code in PR #16878:
URL: https://github.com/apache/pinot/pull/16878#discussion_r2376659968
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotRealtimeTableResource.java:
##########
@@ -395,6 +397,69 @@ public ConsumingSegmentInfoReader.ConsumingSegmentsInfoMap
getConsumingSegmentsI
}
}
+ @GET
+ @Path("/tables/{tableName}/partitionCount")
+ @Authorize(targetType = TargetType.TABLE, paramName = "tableName", action =
Actions.Table.GET_METADATA)
+ @Produces(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Get partition count for a realtime table",
+ notes = "Returns the numbers of consumers for a realtime table by
checking "
+ + "the number of partitions in the table in"
+ + " IdealState")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Success"),
+ @ApiResponse(code = 404, message = "Table not found"),
+ @ApiResponse(code = 500, message = "Internal server error")
+ })
+ public Map<String, Integer> getTablePartitionCount(
+ @ApiParam(value = "Realtime table name with or without type", required =
true, example = "myTable | "
+ + "myTable_REALTIME") @PathParam("tableName") String
realtimeTableName,
+ @Context HttpHeaders headers) {
+ realtimeTableName = DatabaseUtils.translateTableName(realtimeTableName,
headers);
+ try {
+ TableType tableType =
TableNameBuilder.getTableTypeFromTableName(realtimeTableName);
+ if (TableType.OFFLINE == tableType) {
+ throw new IllegalStateException("Cannot get partition count for
OFFLINE table: " + realtimeTableName);
+ }
+ String tableNameWithType =
TableNameBuilder.forType(TableType.REALTIME).tableNameWithType(realtimeTableName);
+ int partitionCount = getPartitionCountFromSegment(tableNameWithType,
_pinotHelixResourceManager);
+
+ Map<String, Integer> response = new HashMap<>();
+ response.put("partitionCount", partitionCount);
+ return response;
+ } catch (Exception e) {
+ throw new ControllerApplicationException(LOGGER,
+ String.format("Failed to get partition count for table %s. %s",
realtimeTableName, e.getMessage()),
+ Response.Status.INTERNAL_SERVER_ERROR, e);
+ }
+ }
+ private int getPartitionCountFromSegment(
+ String tableNameWithType, PinotHelixResourceManager
pinotHelixResourceManager) {
Review Comment:
nit: format by moving first parameter to previous line
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotRealtimeTableResource.java:
##########
@@ -395,6 +397,69 @@ public ConsumingSegmentInfoReader.ConsumingSegmentsInfoMap
getConsumingSegmentsI
}
}
+ @GET
+ @Path("/tables/{tableName}/partitionCount")
+ @Authorize(targetType = TargetType.TABLE, paramName = "tableName", action =
Actions.Table.GET_METADATA)
+ @Produces(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Get partition count for a realtime table",
+ notes = "Returns the numbers of consumers for a realtime table by
checking "
+ + "the number of partitions in the table in"
+ + " IdealState")
Review Comment:
nit: fix formatting - can consolidate in a fewer lines
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotRealtimeTableResource.java:
##########
@@ -395,6 +397,69 @@ public ConsumingSegmentInfoReader.ConsumingSegmentsInfoMap
getConsumingSegmentsI
}
}
+ @GET
+ @Path("/tables/{tableName}/partitionCount")
+ @Authorize(targetType = TargetType.TABLE, paramName = "tableName", action =
Actions.Table.GET_METADATA)
+ @Produces(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Get partition count for a realtime table",
+ notes = "Returns the numbers of consumers for a realtime table by
checking "
+ + "the number of partitions in the table in"
+ + " IdealState")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Success"),
+ @ApiResponse(code = 404, message = "Table not found"),
+ @ApiResponse(code = 500, message = "Internal server error")
+ })
+ public Map<String, Integer> getTablePartitionCount(
+ @ApiParam(value = "Realtime table name with or without type", required =
true, example = "myTable | "
+ + "myTable_REALTIME") @PathParam("tableName") String
realtimeTableName,
+ @Context HttpHeaders headers) {
+ realtimeTableName = DatabaseUtils.translateTableName(realtimeTableName,
headers);
+ try {
+ TableType tableType =
TableNameBuilder.getTableTypeFromTableName(realtimeTableName);
+ if (TableType.OFFLINE == tableType) {
+ throw new IllegalStateException("Cannot get partition count for
OFFLINE table: " + realtimeTableName);
+ }
+ String tableNameWithType =
TableNameBuilder.forType(TableType.REALTIME).tableNameWithType(realtimeTableName);
+ int partitionCount = getPartitionCountFromSegment(tableNameWithType,
_pinotHelixResourceManager);
+
+ Map<String, Integer> response = new HashMap<>();
+ response.put("partitionCount", partitionCount);
+ return response;
+ } catch (Exception e) {
+ throw new ControllerApplicationException(LOGGER,
+ String.format("Failed to get partition count for table %s. %s",
realtimeTableName, e.getMessage()),
+ Response.Status.INTERNAL_SERVER_ERROR, e);
+ }
+ }
+ private int getPartitionCountFromSegment(
+ String tableNameWithType, PinotHelixResourceManager
pinotHelixResourceManager) {
+ IdealState tableIdealState =
pinotHelixResourceManager.getTableIdealState(tableNameWithType);
+ if (tableIdealState == null) {
+ throw new ControllerApplicationException(LOGGER, String.format("Ideal
State for table '%s' is null",
+ tableNameWithType),
+ Response.Status.BAD_REQUEST);
+ }
+ Set<String> consumingSegments =
pinotHelixResourceManager.getConsumingSegments(tableNameWithType);
Review Comment:
why do we need to check for consuming segments first and use that rather
than checking across all? is this just to handle scenarios where partition
count may change?
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotRealtimeTableResource.java:
##########
@@ -395,6 +397,69 @@ public ConsumingSegmentInfoReader.ConsumingSegmentsInfoMap
getConsumingSegmentsI
}
}
+ @GET
+ @Path("/tables/{tableName}/partitionCount")
+ @Authorize(targetType = TargetType.TABLE, paramName = "tableName", action =
Actions.Table.GET_METADATA)
+ @Produces(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Get partition count for a realtime table",
+ notes = "Returns the numbers of consumers for a realtime table by
checking "
+ + "the number of partitions in the table in"
+ + " IdealState")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Success"),
+ @ApiResponse(code = 404, message = "Table not found"),
+ @ApiResponse(code = 500, message = "Internal server error")
+ })
+ public Map<String, Integer> getTablePartitionCount(
+ @ApiParam(value = "Realtime table name with or without type", required =
true, example = "myTable | "
+ + "myTable_REALTIME") @PathParam("tableName") String
realtimeTableName,
+ @Context HttpHeaders headers) {
+ realtimeTableName = DatabaseUtils.translateTableName(realtimeTableName,
headers);
+ try {
+ TableType tableType =
TableNameBuilder.getTableTypeFromTableName(realtimeTableName);
+ if (TableType.OFFLINE == tableType) {
+ throw new IllegalStateException("Cannot get partition count for
OFFLINE table: " + realtimeTableName);
+ }
+ String tableNameWithType =
TableNameBuilder.forType(TableType.REALTIME).tableNameWithType(realtimeTableName);
Review Comment:
nit: add a validateTable() call after this to early return if table doesn't
exist or if it is disabled?
##########
pinot-controller/src/main/java/org/apache/pinot/controller/util/ConsumingSegmentInfoReader.java:
##########
@@ -141,6 +146,18 @@ private String generateServerURL(String tableNameWithType,
String endpoint) {
return String.format("%s/tables/%s/consumingSegmentsInfo", endpoint,
tableNameWithType);
}
+ /**
+ * Calculate partition count from consuming segments using SegmentUtils.
+ */
+ private int getPartitionCount(Set<String> consumingSegments, String
tableNameWithType) {
+ Set<Integer> uniquePartitionIds = consumingSegments.stream()
+ .map(segmentName -> SegmentUtils.getSegmentPartitionId(segmentName,
tableNameWithType,
+ _pinotHelixResourceManager.getHelixZkManager(), null))
+ .filter(Objects::nonNull)
+ .collect(Collectors.toSet());
Review Comment:
this block of code is repeated multiple times. does it make sense to create
a utility function and just call that everywhere so we can keep the logic in
one place?
##########
pinot-controller/src/main/java/org/apache/pinot/controller/util/ConsumingSegmentInfoReader.java:
##########
@@ -195,14 +212,18 @@ static public class ConsumingSegmentsInfoMap {
public int _serversFailingToRespond;
@JsonProperty("serversUnparsableRespond")
public int _serversUnparsableRespond;
+ @JsonProperty("partitionCount")
+ public int _partitionCount;
Review Comment:
since this is getting added here, is there still a need to have a separate
API to fetch just the partition count? just trying to understand from the
workflow perspective
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotRealtimeTableResource.java:
##########
@@ -395,6 +397,69 @@ public ConsumingSegmentInfoReader.ConsumingSegmentsInfoMap
getConsumingSegmentsI
}
}
+ @GET
+ @Path("/tables/{tableName}/partitionCount")
+ @Authorize(targetType = TargetType.TABLE, paramName = "tableName", action =
Actions.Table.GET_METADATA)
+ @Produces(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Get partition count for a realtime table",
+ notes = "Returns the numbers of consumers for a realtime table by
checking "
+ + "the number of partitions in the table in"
+ + " IdealState")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Success"),
+ @ApiResponse(code = 404, message = "Table not found"),
+ @ApiResponse(code = 500, message = "Internal server error")
Review Comment:
update these based on actual possible return codes if needed
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotRealtimeTableResource.java:
##########
@@ -395,6 +397,69 @@ public ConsumingSegmentInfoReader.ConsumingSegmentsInfoMap
getConsumingSegmentsI
}
}
+ @GET
+ @Path("/tables/{tableName}/partitionCount")
+ @Authorize(targetType = TargetType.TABLE, paramName = "tableName", action =
Actions.Table.GET_METADATA)
+ @Produces(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Get partition count for a realtime table",
+ notes = "Returns the numbers of consumers for a realtime table by
checking "
+ + "the number of partitions in the table in"
+ + " IdealState")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Success"),
+ @ApiResponse(code = 404, message = "Table not found"),
+ @ApiResponse(code = 500, message = "Internal server error")
+ })
+ public Map<String, Integer> getTablePartitionCount(
+ @ApiParam(value = "Realtime table name with or without type", required =
true, example = "myTable | "
+ + "myTable_REALTIME") @PathParam("tableName") String
realtimeTableName,
+ @Context HttpHeaders headers) {
+ realtimeTableName = DatabaseUtils.translateTableName(realtimeTableName,
headers);
+ try {
+ TableType tableType =
TableNameBuilder.getTableTypeFromTableName(realtimeTableName);
+ if (TableType.OFFLINE == tableType) {
+ throw new IllegalStateException("Cannot get partition count for
OFFLINE table: " + realtimeTableName);
+ }
+ String tableNameWithType =
TableNameBuilder.forType(TableType.REALTIME).tableNameWithType(realtimeTableName);
+ int partitionCount = getPartitionCountFromSegment(tableNameWithType,
_pinotHelixResourceManager);
+
+ Map<String, Integer> response = new HashMap<>();
+ response.put("partitionCount", partitionCount);
+ return response;
+ } catch (Exception e) {
+ throw new ControllerApplicationException(LOGGER,
+ String.format("Failed to get partition count for table %s. %s",
realtimeTableName, e.getMessage()),
+ Response.Status.INTERNAL_SERVER_ERROR, e);
+ }
+ }
+ private int getPartitionCountFromSegment(
+ String tableNameWithType, PinotHelixResourceManager
pinotHelixResourceManager) {
+ IdealState tableIdealState =
pinotHelixResourceManager.getTableIdealState(tableNameWithType);
+ if (tableIdealState == null) {
+ throw new ControllerApplicationException(LOGGER, String.format("Ideal
State for table '%s' is null",
+ tableNameWithType),
+ Response.Status.BAD_REQUEST);
+ }
Review Comment:
can you keep the return consistent with validateTable()? might be a good
idea to call that as well for your API
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotRealtimeTableResource.java:
##########
@@ -395,6 +397,69 @@ public ConsumingSegmentInfoReader.ConsumingSegmentsInfoMap
getConsumingSegmentsI
}
}
+ @GET
+ @Path("/tables/{tableName}/partitionCount")
+ @Authorize(targetType = TargetType.TABLE, paramName = "tableName", action =
Actions.Table.GET_METADATA)
+ @Produces(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Get partition count for a realtime table",
+ notes = "Returns the numbers of consumers for a realtime table by
checking "
+ + "the number of partitions in the table in"
+ + " IdealState")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Success"),
+ @ApiResponse(code = 404, message = "Table not found"),
+ @ApiResponse(code = 500, message = "Internal server error")
+ })
+ public Map<String, Integer> getTablePartitionCount(
+ @ApiParam(value = "Realtime table name with or without type", required =
true, example = "myTable | "
+ + "myTable_REALTIME") @PathParam("tableName") String
realtimeTableName,
+ @Context HttpHeaders headers) {
+ realtimeTableName = DatabaseUtils.translateTableName(realtimeTableName,
headers);
+ try {
+ TableType tableType =
TableNameBuilder.getTableTypeFromTableName(realtimeTableName);
+ if (TableType.OFFLINE == tableType) {
+ throw new IllegalStateException("Cannot get partition count for
OFFLINE table: " + realtimeTableName);
+ }
+ String tableNameWithType =
TableNameBuilder.forType(TableType.REALTIME).tableNameWithType(realtimeTableName);
+ int partitionCount = getPartitionCountFromSegment(tableNameWithType,
_pinotHelixResourceManager);
+
+ Map<String, Integer> response = new HashMap<>();
+ response.put("partitionCount", partitionCount);
+ return response;
+ } catch (Exception e) {
+ throw new ControllerApplicationException(LOGGER,
+ String.format("Failed to get partition count for table %s. %s",
realtimeTableName, e.getMessage()),
+ Response.Status.INTERNAL_SERVER_ERROR, e);
+ }
+ }
+ private int getPartitionCountFromSegment(
+ String tableNameWithType, PinotHelixResourceManager
pinotHelixResourceManager) {
+ IdealState tableIdealState =
pinotHelixResourceManager.getTableIdealState(tableNameWithType);
+ if (tableIdealState == null) {
+ throw new ControllerApplicationException(LOGGER, String.format("Ideal
State for table '%s' is null",
+ tableNameWithType),
+ Response.Status.BAD_REQUEST);
+ }
+ Set<String> consumingSegments =
pinotHelixResourceManager.getConsumingSegments(tableNameWithType);
Review Comment:
nit: also to avoid duplication of code, can we do something like:
Set<String> segments = consumingSegments.isEmpty() ?
tableIdealState.getPartitionSet() : consumingSegments;
that way you don't have to repeat the same code within the if and else blocks
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
