vvivekiyer commented on code in PR #11578:
URL: https://github.com/apache/pinot/pull/11578#discussion_r1370576854
##########
pinot-core/src/main/java/org/apache/pinot/core/auth/Actions.java:
##########
@@ -85,6 +85,8 @@ public static class Cluster {
public static final String UPDATE_USER = "UpdateUser";
public static final String UPDATE_ZNODE = "UpdateZnode";
public static final String UPLOAD_SEGMENT = "UploadSegment";
+ public static final String GET_INSTANCE_PARTITION = "GetInstancePartition";
Review Comment:
Can we call it `GET_INSTANCE_PARTITIONS` and `UPDATE_INSTANCE_PARTITIONS` to
be consistent with:
1. TableActions
2. REST endpoint names. eg: /tenants/{tenantName}/instancePartitions
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTenantRestletResource.java:
##########
@@ -286,6 +291,75 @@ public String getTablesOnTenant(
}
}
+ @GET
+ @Path("/tenants/{tenantName}/instancePartitions")
+ @Authorize(targetType = TargetType.CLUSTER, action =
Actions.Cluster.GET_INSTANCE_PARTITION)
+ @Authenticate(AccessType.READ)
+ @Produces(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Get the instance partitions of a tenant")
+ @ApiResponses(value = {@ApiResponse(code = 200, message = "Success"),
+ @ApiResponse(code = 500, message = "Instance partitions not found")})
+ public InstancePartitions getInstancePartitions(
+ @ApiParam(value = "Tenant name ", required = true)
@PathParam("tenantName") String tenantName,
+ @ApiParam(value = "instancePartitionType (OFFLINE|CONSUMING|COMPLETED)",
required = true)
+ @QueryParam("instancePartitionType") String instancePartitionType) {
Review Comment:
maybe use `allowableValues = "OFFLINE, CONSUMING, COMPLETED"`
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotInstanceAssignmentRestletResource.java:
##########
@@ -244,20 +244,32 @@ public Map<String, InstancePartitions> assignInstances(
private void assignInstancesForInstancePartitionsType(Map<String,
InstancePartitions> instancePartitionsMap,
TableConfig tableConfig, List<InstanceConfig> instanceConfigs,
InstancePartitionsType instancePartitionsType) {
String tableNameWithType = tableConfig.getTableName();
- if (TableConfigUtils.hasPreConfiguredInstancePartitions(tableConfig,
instancePartitionsType)) {
+ if (!TableConfigUtils.hasPreConfiguredInstancePartitions(tableConfig,
instancePartitionsType)) {
Review Comment:
If a tableConfig doesn't have the InstancePartitionMap configured, we will
always come into this if block.
That means we'll never reach the "else if" block on L254 if
instancePartitionMap is not configured.
Is that the right behavior?
My understanding was that we can use PRE_CONFIGURATION_INSTANCE_SELECTOR as
long as InstanceAssignmentConfigMap in tableConfig is set.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTenantRestletResource.java:
##########
@@ -286,6 +291,75 @@ public String getTablesOnTenant(
}
}
+ @GET
+ @Path("/tenants/{tenantName}/instancePartitions")
+ @Authorize(targetType = TargetType.CLUSTER, action =
Actions.Cluster.GET_INSTANCE_PARTITION)
+ @Authenticate(AccessType.READ)
+ @Produces(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Get the instance partitions of a tenant")
+ @ApiResponses(value = {@ApiResponse(code = 200, message = "Success"),
+ @ApiResponse(code = 500, message = "Instance partitions not found")})
+ public InstancePartitions getInstancePartitions(
+ @ApiParam(value = "Tenant name ", required = true)
@PathParam("tenantName") String tenantName,
+ @ApiParam(value = "instancePartitionType (OFFLINE|CONSUMING|COMPLETED)",
required = true)
+ @QueryParam("instancePartitionType") String instancePartitionType) {
+ String tenantNameWithType =
InstancePartitionsType.valueOf(instancePartitionType)
+ .getInstancePartitionsName(tenantName);
+ InstancePartitions instancePartitions =
+
InstancePartitionsUtils.fetchInstancePartitions(_pinotHelixResourceManager.getPropertyStore(),
+ tenantNameWithType);
+
+ if (instancePartitions == null) {
+ throw new ControllerApplicationException(LOGGER, "Failed to find the
instance partitions",
Review Comment:
nit: log the tenantNameWithType as well here.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTenantRestletResource.java:
##########
@@ -286,6 +291,75 @@ public String getTablesOnTenant(
}
}
+ @GET
+ @Path("/tenants/{tenantName}/instancePartitions")
+ @Authorize(targetType = TargetType.CLUSTER, action =
Actions.Cluster.GET_INSTANCE_PARTITION)
+ @Authenticate(AccessType.READ)
+ @Produces(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Get the instance partitions of a tenant")
+ @ApiResponses(value = {@ApiResponse(code = 200, message = "Success"),
+ @ApiResponse(code = 500, message = "Instance partitions not found")})
+ public InstancePartitions getInstancePartitions(
+ @ApiParam(value = "Tenant name ", required = true)
@PathParam("tenantName") String tenantName,
+ @ApiParam(value = "instancePartitionType (OFFLINE|CONSUMING|COMPLETED)",
required = true)
+ @QueryParam("instancePartitionType") String instancePartitionType) {
+ String tenantNameWithType =
InstancePartitionsType.valueOf(instancePartitionType)
+ .getInstancePartitionsName(tenantName);
+ InstancePartitions instancePartitions =
+
InstancePartitionsUtils.fetchInstancePartitions(_pinotHelixResourceManager.getPropertyStore(),
+ tenantNameWithType);
+
+ if (instancePartitions == null) {
+ throw new ControllerApplicationException(LOGGER, "Failed to find the
instance partitions",
+ Response.Status.NOT_FOUND);
+ } else {
+ return instancePartitions;
+ }
+ }
+
+ @PUT
+ @Path("/tenants/{tenantName}/instancePartitions")
+ @Authorize(targetType = TargetType.CLUSTER, action =
Actions.Cluster.UPDATE_INSTANCE_PARTITION)
+ @Authenticate(AccessType.UPDATE)
+ @Consumes(MediaType.APPLICATION_JSON)
+ @Produces(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Update an instance partition for a server type in a
tenant")
+ @ApiResponses(value = {@ApiResponse(code = 200, message = "Success"),
+ @ApiResponse(code = 500, message = "Failed to update the tenant")})
+ public InstancePartitions assignInstancesPartitionMap(
+ @ApiParam(value = "Tenant name ", required = true)
@PathParam("tenantName") String tenantName,
+ @ApiParam(value = "instancePartitionType (OFFLINE|CONSUMING|COMPLETED)",
required = true)
+ @QueryParam("instancePartitionType") String instancePartitionType,
+ String instancePartitionsStr) {
Review Comment:
Let's make this required?
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/assignment/InstanceAssignmentConfig.java:
##########
@@ -82,6 +82,7 @@ public InstanceReplicaGroupPartitionConfig
getReplicaGroupPartitionConfig() {
}
public enum PartitionSelector {
- FD_AWARE_INSTANCE_PARTITION_SELECTOR,
INSTANCE_REPLICA_GROUP_PARTITION_SELECTOR
+ FD_AWARE_INSTANCE_PARTITION_SELECTOR,
INSTANCE_REPLICA_GROUP_PARTITION_SELECTOR,
+ PRE_CONFIGURATION_BASED_PARTITION_SELECTOR
Review Comment:
We already use the name `PreConfiguredInstancePartitions` for
instancePartitionsMap tableConfig. To avoid confusion, can we give this a
better name?
Maybe `PRE_DEFINED_CONFIG_PARTITION_SELECTOR` or
`MIRROR_SERVER_BASED_PARTITION_SELECTOR`?
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTenantRestletResource.java:
##########
@@ -286,6 +291,75 @@ public String getTablesOnTenant(
}
}
+ @GET
+ @Path("/tenants/{tenantName}/instancePartitions")
+ @Authorize(targetType = TargetType.CLUSTER, action =
Actions.Cluster.GET_INSTANCE_PARTITION)
+ @Authenticate(AccessType.READ)
+ @Produces(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Get the instance partitions of a tenant")
+ @ApiResponses(value = {@ApiResponse(code = 200, message = "Success"),
+ @ApiResponse(code = 500, message = "Instance partitions not found")})
Review Comment:
IMO, we should not be returning a 500 when - Instance Partitions are not
found. 500 indicates there was an internal error reading the configs. Something
like 404 is more appropriate.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTenantRestletResource.java:
##########
@@ -286,6 +291,75 @@ public String getTablesOnTenant(
}
}
+ @GET
+ @Path("/tenants/{tenantName}/instancePartitions")
+ @Authorize(targetType = TargetType.CLUSTER, action =
Actions.Cluster.GET_INSTANCE_PARTITION)
+ @Authenticate(AccessType.READ)
+ @Produces(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Get the instance partitions of a tenant")
+ @ApiResponses(value = {@ApiResponse(code = 200, message = "Success"),
+ @ApiResponse(code = 500, message = "Instance partitions not found")})
+ public InstancePartitions getInstancePartitions(
+ @ApiParam(value = "Tenant name ", required = true)
@PathParam("tenantName") String tenantName,
+ @ApiParam(value = "instancePartitionType (OFFLINE|CONSUMING|COMPLETED)",
required = true)
+ @QueryParam("instancePartitionType") String instancePartitionType) {
+ String tenantNameWithType =
InstancePartitionsType.valueOf(instancePartitionType)
+ .getInstancePartitionsName(tenantName);
+ InstancePartitions instancePartitions =
+
InstancePartitionsUtils.fetchInstancePartitions(_pinotHelixResourceManager.getPropertyStore(),
+ tenantNameWithType);
+
+ if (instancePartitions == null) {
+ throw new ControllerApplicationException(LOGGER, "Failed to find the
instance partitions",
+ Response.Status.NOT_FOUND);
+ } else {
+ return instancePartitions;
+ }
+ }
+
+ @PUT
+ @Path("/tenants/{tenantName}/instancePartitions")
+ @Authorize(targetType = TargetType.CLUSTER, action =
Actions.Cluster.UPDATE_INSTANCE_PARTITION)
+ @Authenticate(AccessType.UPDATE)
+ @Consumes(MediaType.APPLICATION_JSON)
+ @Produces(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Update an instance partition for a server type in a
tenant")
+ @ApiResponses(value = {@ApiResponse(code = 200, message = "Success"),
+ @ApiResponse(code = 500, message = "Failed to update the tenant")})
+ public InstancePartitions assignInstancesPartitionMap(
+ @ApiParam(value = "Tenant name ", required = true)
@PathParam("tenantName") String tenantName,
+ @ApiParam(value = "instancePartitionType (OFFLINE|CONSUMING|COMPLETED)",
required = true)
+ @QueryParam("instancePartitionType") String instancePartitionType,
+ String instancePartitionsStr) {
+ InstancePartitions instancePartitions;
+ try {
+ instancePartitions = JsonUtils.stringToObject(instancePartitionsStr,
InstancePartitions.class);
+ } catch (IOException e) {
+ throw new ControllerApplicationException(LOGGER, "Failed to deserialize
the instance partitions",
+ Response.Status.BAD_REQUEST);
+ }
+
+ String tenantNameWithType =
InstancePartitionsType.valueOf(instancePartitionType)
+ .getInstancePartitionsName(tenantName);
+
Preconditions.checkState(instancePartitions.getInstancePartitionsName().equals(tenantNameWithType),
Review Comment:
Wouldn't recommend a Precondition.check here as it is a user provided error
and a recoverable error. So, we should return BAD_REQUEST error to the user.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotInstanceAssignmentRestletResource.java:
##########
@@ -244,20 +244,32 @@ public Map<String, InstancePartitions> assignInstances(
private void assignInstancesForInstancePartitionsType(Map<String,
InstancePartitions> instancePartitionsMap,
TableConfig tableConfig, List<InstanceConfig> instanceConfigs,
InstancePartitionsType instancePartitionsType) {
String tableNameWithType = tableConfig.getTableName();
- if (TableConfigUtils.hasPreConfiguredInstancePartitions(tableConfig,
instancePartitionsType)) {
+ if (!TableConfigUtils.hasPreConfiguredInstancePartitions(tableConfig,
instancePartitionsType)) {
+ InstancePartitions existingInstancePartitions =
+
InstancePartitionsUtils.fetchInstancePartitions(_resourceManager.getHelixZkManager().getHelixPropertyStore(),
+
InstancePartitionsUtils.getInstancePartitionsName(tableNameWithType,
instancePartitionsType.toString()));
+ instancePartitionsMap.put(instancePartitionsType.toString(),
+ new
InstanceAssignmentDriver(tableConfig).assignInstances(instancePartitionsType,
instanceConfigs,
+ existingInstancePartitions));
+ } else if
(InstanceAssignmentConfigUtils.isPreConfigurationBasedAssignment(tableConfig,
instancePartitionsType)) {
+ InstancePartitions existingInstancePartitions =
+
InstancePartitionsUtils.fetchInstancePartitions(_resourceManager.getHelixZkManager().getHelixPropertyStore(),
+
InstancePartitionsUtils.getInstancePartitionsName(tableNameWithType,
instancePartitionsType.toString()));
+ String rawTableName =
TableNameBuilder.extractRawTableName(tableNameWithType);
+ InstancePartitions preConfigured =
Review Comment:
The tenant level pre_configuration is available under the
tenantNameWithType, correct? Over here, we seem to be working with the
tableName. Is that the expectation?
Maybe adding a comment in this function about the intended behavior will
help the readers understand better as we are dealing with tenant-level instance
partitions configs and table level instance partition configs.
--
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
