Copilot commented on code in PR #17627:
URL: https://github.com/apache/pinot/pull/17627#discussion_r2768442274
##########
pinot-controller/src/main/java/org/apache/pinot/controller/services/PinotTableReloadService.java:
##########
@@ -106,27 +112,53 @@ public SuccessResponse reloadSegment(String tableName,
String segmentName, boole
}
public SuccessResponse reloadAllSegments(String tableName, String
tableTypeStr, boolean forceDownload,
- String targetInstance, String instanceToSegmentsMapInJson, HttpHeaders
headers)
+ String targetInstance, String instanceToSegmentsMapInJson, String
startTimestampStr, String endTimestampStr,
+ boolean excludeOverlapping, HttpHeaders headers)
throws IOException {
tableName = DatabaseUtils.translateTableName(tableName, headers);
TableType tableTypeFromTableName =
TableNameBuilder.getTableTypeFromTableName(tableName);
TableType tableTypeFromRequest = Constants.validateTableType(tableTypeStr);
- // When rawTableName is provided but w/o table type, Pinot tries to reload
both OFFLINE
+ // When rawTableName is provided but without table type, Pinot tries to
reload both OFFLINE
// and REALTIME tables for the raw table. But forceDownload option only
works with
// OFFLINE table currently, so we limit the table type to OFFLINE to let
Pinot continue
- // to reload w/o being accidentally aborted upon REALTIME table type.
+ // to reload without being accidentally aborted upon REALTIME table type.
// TODO: support to force download immutable segments from RealTime table.
if (forceDownload && (tableTypeFromTableName == null &&
tableTypeFromRequest == null)) {
tableTypeFromRequest = TableType.OFFLINE;
}
+ boolean hasStartTimestamp = !Strings.isNullOrEmpty(startTimestampStr);
+ boolean hasEndTimestamp = !Strings.isNullOrEmpty(endTimestampStr);
+ if (hasStartTimestamp || hasEndTimestamp) {
+ if (!(hasStartTimestamp && hasEndTimestamp)) {
+ throw new ControllerApplicationException(LOG, "startTimestamp and
endTimestamp must be provided together.",
+ Response.Status.BAD_REQUEST);
+ }
+ if (targetInstance != null || instanceToSegmentsMapInJson != null) {
+ throw new ControllerApplicationException(LOG,
+ "startTimestamp/endTimestamp/excludeOverlapping cannot be used
with targetInstance/instanceToSegmentsMap.",
+ Response.Status.BAD_REQUEST);
+ }
Review Comment:
This validation is duplicated in the reloadSegmentsInTimeRange method at
lines 207-209. Consider removing the duplicate check since
reloadSegmentsInTimeRange is called immediately after this validation at line
141, making the redundant check unnecessary.
```suggestion
```
##########
pinot-controller/src/main/java/org/apache/pinot/controller/services/PinotTableReloadService.java:
##########
@@ -169,6 +201,108 @@ public SuccessResponse reloadAllSegments(String
tableName, String tableTypeStr,
return new SuccessResponse(JsonUtils.objectToString(perTableMsgData));
}
+ public SuccessResponse reloadSegmentsInTimeRange(String tableName, String
tableTypeStr, String startTimestampStr,
+ String endTimestampStr, boolean excludeOverlapping, boolean
forceDownload, @Nullable String targetInstance,
+ HttpHeaders headers) {
+ if (Strings.isNullOrEmpty(startTimestampStr) ||
Strings.isNullOrEmpty(endTimestampStr)) {
+ throw new ControllerApplicationException(LOG, "startTimestamp and
endTimestamp must be provided.",
+ Response.Status.BAD_REQUEST);
+ }
+ long startTimestamp;
+ long endTimestamp;
+ try {
+ startTimestamp = Long.parseLong(startTimestampStr);
+ endTimestamp = Long.parseLong(endTimestampStr);
+ } catch (NumberFormatException e) {
+ throw new ControllerApplicationException(LOG,
+ "Failed to parse the start/end timestamp. Please make sure they are
in 'milliseconds since epoch' format.",
+ Response.Status.BAD_REQUEST, e);
+ }
+ if (startTimestamp >= endTimestamp) {
Review Comment:
Missing validation for the same condition in the reloadAllSegments method.
When time range parameters are provided, the validation at line 221 ensures
startTimestamp < endTimestamp, but this check is not performed in
reloadAllSegments before delegating to reloadSegmentsInTimeRange.
--
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]
