Jackie-Jiang commented on code in PR #9413:
URL: https://github.com/apache/pinot/pull/9413#discussion_r982973441
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentRestletResource.java:
##########
@@ -1035,4 +1037,52 @@ public
ConsumingSegmentInfoReader.ConsumingSegmentsInfoMap getConsumingSegmentsI
Response.Status.INTERNAL_SERVER_ERROR, e);
}
}
+
+ @POST
+ @Path("/segments/{tableNameWithType}/updateZKTimeInterval")
+ @Authenticate(AccessType.UPDATE)
+ @Produces(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Update the start and end time of the segments based
on latest schema",
+ notes = "Update the start and end time of the segments based on latest
schema")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Success"),
+ @ApiResponse(code = 404, message = "Table not found"),
+ @ApiResponse(code = 500, message = "Internal server error")
+ })
+ public SuccessResponse updateTimeIntervalZK(
+ @ApiParam(value = "Table name with type", required = true,
+ example = "myTable_REALTIME") @PathParam("tableNameWithType") String
tableNameWithType) {
+ TableType tableType =
TableNameBuilder.getTableTypeFromTableName(tableNameWithType);
+ if (tableType == null) {
+ throw new ControllerApplicationException(LOGGER,
+ String.format("Table type not provided with table name %s",
tableNameWithType),
+ Response.Status.INTERNAL_SERVER_ERROR);
Review Comment:
This should be `BAD_REQUEST`
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentRestletResource.java:
##########
@@ -1035,4 +1037,52 @@ public
ConsumingSegmentInfoReader.ConsumingSegmentsInfoMap getConsumingSegmentsI
Response.Status.INTERNAL_SERVER_ERROR, e);
}
}
+
+ @POST
+ @Path("/segments/{tableNameWithType}/updateZKTimeInterval")
+ @Authenticate(AccessType.UPDATE)
+ @Produces(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Update the start and end time of the segments based
on latest schema",
+ notes = "Update the start and end time of the segments based on latest
schema")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Success"),
+ @ApiResponse(code = 404, message = "Table not found"),
+ @ApiResponse(code = 500, message = "Internal server error")
+ })
+ public SuccessResponse updateTimeIntervalZK(
+ @ApiParam(value = "Table name with type", required = true,
+ example = "myTable_REALTIME") @PathParam("tableNameWithType") String
tableNameWithType) {
+ TableType tableType =
TableNameBuilder.getTableTypeFromTableName(tableNameWithType);
+ if (tableType == null) {
+ throw new ControllerApplicationException(LOGGER,
+ String.format("Table type not provided with table name %s",
tableNameWithType),
+ Response.Status.INTERNAL_SERVER_ERROR);
+ }
+ return updateZKTimeIntervalInternal(tableNameWithType);
+ }
+
+ /**
+ * Internal method to update schema
+ * @param tableNameWithType name of the table
+ * @return
+ */
+ private SuccessResponse updateZKTimeIntervalInternal(String
tableNameWithType) {
+ TableConfig tableConfig =
_pinotHelixResourceManager.getTableConfig(tableNameWithType);
+ Preconditions.checkState(tableConfig != null, "Failed to find table
config for table: {}", tableNameWithType);
+
+ Schema tableSchema =
_pinotHelixResourceManager.getTableSchema(tableNameWithType);
+ Preconditions.checkState(tableSchema != null, "Failed to find schema for
table: {}", tableNameWithType);
+
+ String schemaName = tableSchema.getSchemaName();
+ try {
+ _pinotHelixResourceManager.updateSegmentsZKTimeInterval(tableConfig,
tableSchema);
+ } catch (Exception e) {
+ throw new ControllerApplicationException(LOGGER,
+ String.format("Failed to update time interval zk metadata for
table %s, exception: %s", tableNameWithType,
Review Comment:
(minor) No need to include the exception message since we already have the
exception wrapped inside
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentRestletResource.java:
##########
@@ -1035,4 +1037,52 @@ public
ConsumingSegmentInfoReader.ConsumingSegmentsInfoMap getConsumingSegmentsI
Response.Status.INTERNAL_SERVER_ERROR, e);
}
}
+
+ @POST
+ @Path("/segments/{tableNameWithType}/updateZKTimeInterval")
+ @Authenticate(AccessType.UPDATE)
+ @Produces(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Update the start and end time of the segments based
on latest schema",
+ notes = "Update the start and end time of the segments based on latest
schema")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Success"),
+ @ApiResponse(code = 404, message = "Table not found"),
+ @ApiResponse(code = 500, message = "Internal server error")
+ })
+ public SuccessResponse updateTimeIntervalZK(
+ @ApiParam(value = "Table name with type", required = true,
+ example = "myTable_REALTIME") @PathParam("tableNameWithType") String
tableNameWithType) {
+ TableType tableType =
TableNameBuilder.getTableTypeFromTableName(tableNameWithType);
+ if (tableType == null) {
+ throw new ControllerApplicationException(LOGGER,
+ String.format("Table type not provided with table name %s",
tableNameWithType),
+ Response.Status.INTERNAL_SERVER_ERROR);
+ }
+ return updateZKTimeIntervalInternal(tableNameWithType);
+ }
+
+ /**
+ * Internal method to update schema
+ * @param tableNameWithType name of the table
+ * @return
+ */
+ private SuccessResponse updateZKTimeIntervalInternal(String
tableNameWithType) {
+ TableConfig tableConfig =
_pinotHelixResourceManager.getTableConfig(tableNameWithType);
+ Preconditions.checkState(tableConfig != null, "Failed to find table
config for table: {}", tableNameWithType);
Review Comment:
```suggestion
if (tableConfig == null) {
throw new ControllerApplicationException(LOGGER, "Failed to find
table config for table: " + tableNameWithType), Response.Status.NOT_FOUND);
}
```
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentRestletResource.java:
##########
@@ -1035,4 +1037,52 @@ public
ConsumingSegmentInfoReader.ConsumingSegmentsInfoMap getConsumingSegmentsI
Response.Status.INTERNAL_SERVER_ERROR, e);
}
}
+
+ @POST
+ @Path("/segments/{tableNameWithType}/updateZKTimeInterval")
+ @Authenticate(AccessType.UPDATE)
+ @Produces(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Update the start and end time of the segments based
on latest schema",
+ notes = "Update the start and end time of the segments based on latest
schema")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Success"),
+ @ApiResponse(code = 404, message = "Table not found"),
+ @ApiResponse(code = 500, message = "Internal server error")
+ })
+ public SuccessResponse updateTimeIntervalZK(
+ @ApiParam(value = "Table name with type", required = true,
+ example = "myTable_REALTIME") @PathParam("tableNameWithType") String
tableNameWithType) {
+ TableType tableType =
TableNameBuilder.getTableTypeFromTableName(tableNameWithType);
+ if (tableType == null) {
+ throw new ControllerApplicationException(LOGGER,
+ String.format("Table type not provided with table name %s",
tableNameWithType),
+ Response.Status.INTERNAL_SERVER_ERROR);
+ }
+ return updateZKTimeIntervalInternal(tableNameWithType);
+ }
+
+ /**
+ * Internal method to update schema
+ * @param tableNameWithType name of the table
+ * @return
+ */
+ private SuccessResponse updateZKTimeIntervalInternal(String
tableNameWithType) {
+ TableConfig tableConfig =
_pinotHelixResourceManager.getTableConfig(tableNameWithType);
+ Preconditions.checkState(tableConfig != null, "Failed to find table
config for table: {}", tableNameWithType);
+
+ Schema tableSchema =
_pinotHelixResourceManager.getTableSchema(tableNameWithType);
+ Preconditions.checkState(tableSchema != null, "Failed to find schema for
table: {}", tableNameWithType);
+
+ String schemaName = tableSchema.getSchemaName();
+ try {
+ _pinotHelixResourceManager.updateSegmentsZKTimeInterval(tableConfig,
tableSchema);
Review Comment:
Let's check the existence of time column, and get the `DateTimeFieldSpec`
here. Throw proper exception (user error) when time column doesn't exist in
table config, or `DateTimeFieldSpec` doesn't exist in schema. Only pass table
name and dateTimeFieldSpec to the method
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentRestletResource.java:
##########
@@ -1035,4 +1037,52 @@ public
ConsumingSegmentInfoReader.ConsumingSegmentsInfoMap getConsumingSegmentsI
Response.Status.INTERNAL_SERVER_ERROR, e);
}
}
+
+ @POST
+ @Path("/segments/{tableNameWithType}/updateZKTimeInterval")
+ @Authenticate(AccessType.UPDATE)
+ @Produces(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Update the start and end time of the segments based
on latest schema",
+ notes = "Update the start and end time of the segments based on latest
schema")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Success"),
+ @ApiResponse(code = 404, message = "Table not found"),
+ @ApiResponse(code = 500, message = "Internal server error")
+ })
+ public SuccessResponse updateTimeIntervalZK(
+ @ApiParam(value = "Table name with type", required = true,
+ example = "myTable_REALTIME") @PathParam("tableNameWithType") String
tableNameWithType) {
+ TableType tableType =
TableNameBuilder.getTableTypeFromTableName(tableNameWithType);
+ if (tableType == null) {
+ throw new ControllerApplicationException(LOGGER,
+ String.format("Table type not provided with table name %s",
tableNameWithType),
+ Response.Status.INTERNAL_SERVER_ERROR);
+ }
+ return updateZKTimeIntervalInternal(tableNameWithType);
+ }
+
+ /**
+ * Internal method to update schema
+ * @param tableNameWithType name of the table
+ * @return
+ */
+ private SuccessResponse updateZKTimeIntervalInternal(String
tableNameWithType) {
+ TableConfig tableConfig =
_pinotHelixResourceManager.getTableConfig(tableNameWithType);
+ Preconditions.checkState(tableConfig != null, "Failed to find table
config for table: {}", tableNameWithType);
+
+ Schema tableSchema =
_pinotHelixResourceManager.getTableSchema(tableNameWithType);
+ Preconditions.checkState(tableSchema != null, "Failed to find schema for
table: {}", tableNameWithType);
Review Comment:
Same here
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java:
##########
@@ -2265,6 +2279,18 @@ private void deleteTableOnServer(String
tableNameWithType) {
}
}
+ public void updateZkTimeInterval(TableConfig tableConfig, SegmentZKMetadata
segmentZKMetadata,
+ Schema newSchema) {
+ String segmentName = segmentZKMetadata.getSegmentName();
+
+ String timeColumnName =
tableConfig.getValidationConfig().getTimeColumnName();
+ if (StringUtils.isNotEmpty(timeColumnName)) {
+ DateTimeFieldSpec dateTimeFieldSpec =
newSchema.getDateTimeSpec(timeColumnName);
Review Comment:
We should use `getSpecForTimeColumn(timeColumnName)` which can handle schema
in old format
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java:
##########
@@ -2265,6 +2279,18 @@ private void deleteTableOnServer(String
tableNameWithType) {
}
}
+ public void updateZkTimeInterval(TableConfig tableConfig, SegmentZKMetadata
segmentZKMetadata,
+ Schema newSchema) {
+ String segmentName = segmentZKMetadata.getSegmentName();
+
+ String timeColumnName =
tableConfig.getValidationConfig().getTimeColumnName();
+ if (StringUtils.isNotEmpty(timeColumnName)) {
+ DateTimeFieldSpec dateTimeFieldSpec =
newSchema.getDateTimeSpec(timeColumnName);
+ ZKMetadataUtils.updateSegmentZKTimeInterval(segmentZKMetadata,
dateTimeFieldSpec);
+ LOGGER.info("Updated segment zookeeper metadata: {} of table: {}",
segmentName, tableConfig.getTableName());
Review Comment:
Remove this since it can flood the log
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java:
##########
@@ -1284,6 +1285,19 @@ public void addSchema(Schema schema, boolean override,
boolean force)
}
}
+ public void updateSegmentsZKTimeInterval(TableConfig tableConfig, Schema
schema) {
+ String schemaName = schema.getSchemaName();
+ LOGGER.info("Updating segment zookeeper time interval metadata: {}",
schemaName);
Review Comment:
Log the table name instead of the schema name
```suggestion
LOGGER.info("Updating segment time interval in ZK metadata for table:
{}", tableConfig.getTableName());
```
--
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]
