Copilot commented on code in PR #17627:
URL: https://github.com/apache/pinot/pull/17627#discussion_r2767889782
##########
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 'millisSinceEpoch' format.",
Review Comment:
The error message uses 'millisSinceEpoch' format specification which is
somewhat non-standard. Consider using 'epoch milliseconds' or 'milliseconds
since epoch' which are more commonly used terms in the industry.
```suggestion
"Failed to parse the start/end timestamp. Please make sure they
are in 'milliseconds since epoch' format.",
```
##########
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 'millisSinceEpoch' 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);
+ }
+
+ tableName = DatabaseUtils.translateTableName(tableName, headers);
+ TableType tableTypeFromTableName =
TableNameBuilder.getTableTypeFromTableName(tableName);
+ TableType tableTypeFromRequest = Constants.validateTableType(tableTypeStr);
+ // 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 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;
+ }
+ List<String> tableNamesWithType =
+
ResourceUtils.getExistingTableNamesWithType(_pinotHelixResourceManager,
tableName, tableTypeFromRequest, LOG);
+ Map<String, Map<String, String>> perTableMsgData = new LinkedHashMap<>();
+ for (String tableNameWithType : tableNamesWithType) {
+ List<String> segments =
+ _pinotHelixResourceManager.getSegmentsFor(tableNameWithType, true,
startTimestamp, endTimestamp,
+ excludeOverlapping);
+ if (segments.isEmpty()) {
+ continue;
+ }
+ Set<String> selectedSegments = new HashSet<>(segments);
+ Map<String, List<String>> serverToSegmentsMap =
+ _pinotHelixResourceManager.getServerToSegmentsMap(tableNameWithType,
targetInstance, false);
+ Map<String, List<String>> filteredInstanceToSegmentsMap = new
HashMap<>();
+ for (Map.Entry<String, List<String>> entry :
serverToSegmentsMap.entrySet()) {
+ List<String> instanceSegments =
+
entry.getValue().stream().filter(selectedSegments::contains).collect(Collectors.toList());
+ if (!instanceSegments.isEmpty()) {
+ filteredInstanceToSegmentsMap.put(entry.getKey(), instanceSegments);
+ }
+ }
+ if (filteredInstanceToSegmentsMap.isEmpty()) {
+ continue;
+ }
+ String reloadJobId = UUID.randomUUID().toString();
+ long startTimeMs = System.currentTimeMillis();
+ Map<String, Pair<Integer, String>> instanceMsgInfoMap =
+ _pinotHelixResourceManager.reloadSegments(tableNameWithType,
forceDownload, filteredInstanceToSegmentsMap,
+ reloadJobId);
+ int numReloadMsgSent =
instanceMsgInfoMap.values().stream().mapToInt(Pair::getLeft).sum();
+ if (numReloadMsgSent <= 0) {
+ continue;
+ }
+ Set<String> segmentsToReload =
filteredInstanceToSegmentsMap.values().stream()
+ .flatMap(List::stream)
+ .collect(Collectors.toSet());
+ String segmentNames =
+ StringUtils.join(segmentsToReload,
SegmentNameUtils.SEGMENT_NAME_SEPARATOR);
+ Map<String, String> tableReloadMeta = new HashMap<>();
+ tableReloadMeta.put("numMessagesSent", String.valueOf(numReloadMsgSent));
+ tableReloadMeta.put("reloadJobId", reloadJobId);
+ perTableMsgData.put(tableNameWithType, tableReloadMeta);
+ try {
+ if
(_pinotHelixResourceManager.addNewReloadSegmentJob(tableNameWithType,
segmentNames, targetInstance,
Review Comment:
The variable name `segmentNames` is misleading as it contains a single
concatenated string, not multiple segment names. Consider renaming to
`segmentNamesStr` or `concatenatedSegmentNames` to better reflect its content.
```suggestion
String segmentNamesStr =
StringUtils.join(segmentsToReload,
SegmentNameUtils.SEGMENT_NAME_SEPARATOR);
Map<String, String> tableReloadMeta = new HashMap<>();
tableReloadMeta.put("numMessagesSent",
String.valueOf(numReloadMsgSent));
tableReloadMeta.put("reloadJobId", reloadJobId);
perTableMsgData.put(tableNameWithType, tableReloadMeta);
try {
if
(_pinotHelixResourceManager.addNewReloadSegmentJob(tableNameWithType,
segmentNamesStr, targetInstance,
```
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableReloadResource.java:
##########
@@ -128,7 +128,9 @@ public SuccessResponse reloadSegment(
@Authenticate(AccessType.UPDATE)
@Produces(MediaType.APPLICATION_JSON)
@ApiOperation(value = "Reload all segments in a table",
- notes = "Reloads all segments for the specified table. Supports
filtering by type, instance, or custom mapping.")
+ notes = "Reloads all segments for the specified table. Supports
filtering by type, instance, "
+ + "custom mapping, or time range. Time range params are in
milliseconds and the range is "
+ + "[startTimestamp, endTimestamp).")
Review Comment:
The documentation should clarify that when using time range parameters,
`targetInstance` and `instanceToSegmentsMap` cannot be used simultaneously, as
this constraint exists in the implementation but isn't documented in the API
notes.
```suggestion
+ "[startTimestamp, endTimestamp). When using time range
parameters, do not specify "
+ "targetInstance or instanceToSegmentsMap; these options are
mutually exclusive with time "
+ "range filters.")
```
--
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]
