Copilot commented on code in PR #17627:
URL: https://github.com/apache/pinot/pull/17627#discussion_r2768870544
##########
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);
+ }
+ return reloadSegmentsInTimeRange(tableName, tableTypeStr,
startTimestampStr, endTimestampStr,
+ excludeOverlapping, forceDownload, null, headers);
+ } else if (excludeOverlapping) {
+ throw new ControllerApplicationException(LOG,
+ "excludeOverlapping can only be used together with
startTimestamp/endTimestamp.",
+ Response.Status.BAD_REQUEST);
+ }
List<String> tableNamesWithType =
ResourceUtils.getExistingTableNamesWithType(_pinotHelixResourceManager,
tableName, tableTypeFromRequest, LOG);
if (instanceToSegmentsMapInJson != null) {
Map<String, List<String>> instanceToSegmentsMap =
JsonUtils.stringToObject(instanceToSegmentsMapInJson, new
TypeReference<>() {
});
- Map<String, Map<String, Map<String, String>>> tableInstanceMsgData =
- reloadSegments(tableNamesWithType, forceDownload,
instanceToSegmentsMap);
+ Map<String, Map<String, Map<String, String>>> tableInstanceMsgData = new
LinkedHashMap<>();
+ for (String tableNameWithType : tableNamesWithType) {
+ Map<String, Map<String, String>> instanceMsgData =
+ reloadSegmentsForTable(tableNameWithType, forceDownload,
instanceToSegmentsMap);
+ if (!instanceMsgData.isEmpty()) {
+ tableInstanceMsgData.put(tableNameWithType, instanceMsgData);
+ }
+ }
Review Comment:
The newly extracted `reloadSegmentsForTable` method lacks dedicated test
coverage. While it's indirectly tested through the existing reload flow, adding
explicit tests would improve coverage of its error handling and metadata
storage logic.
##########
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) {
+ throw new ControllerApplicationException(LOG, String.format(
+ "startTimestamp must be less than endTimestamp. Provided: start=%d,
end=%d", startTimestamp, endTimestamp),
+ Response.Status.BAD_REQUEST);
+ }
Review Comment:
The validation that rejects cases where `startTimestamp >= endTimestamp` is
not covered by the existing tests. Add a test case to verify this boundary
condition is properly handled.
--
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]
