Copilot commented on code in PR #17627:
URL: https://github.com/apache/pinot/pull/17627#discussion_r3032512245
##########
pinot-controller/src/main/java/org/apache/pinot/controller/services/PinotTableReloadService.java:
##########
@@ -169,6 +194,97 @@ 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) {
+ 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);
+ }
+
+ tableName = DatabaseUtils.translateTableName(tableName, headers);
+ TableType tableTypeFromRequest = resolveTableTypeForReload(tableName,
tableTypeStr, forceDownload);
+ 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());
Review Comment:
`reloadSegmentsInTimeRange` builds `serverToSegmentsMap` for the whole table
via `getServerToSegmentsMap(...)` and then filters it down to the selected
segments. For large tables this can be expensive (full ideal-state scan + large
in-memory map) and partially undermines the goal of reloading a small subset.
Consider adding/using a HelixResourceManager helper that derives the
per-instance map directly for the provided segment set (e.g., single IdealState
fetch, iterate only target segments), or at least documenting this cost with a
TODO here like in the status reporter.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/services/PinotTableReloadService.java:
##########
@@ -169,6 +194,97 @@ 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) {
+ 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);
+ }
+
+ tableName = DatabaseUtils.translateTableName(tableName, headers);
+ TableType tableTypeFromRequest = resolveTableTypeForReload(tableName,
tableTypeStr, forceDownload);
+ 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);
Review Comment:
The `excludeOverlapping` behavior in time-range reload is new/updated
(passed through to `getSegmentsFor(...)`), but there is no unit test exercising
`excludeOverlapping=true` vs `false`. Adding a test that asserts the flag is
forwarded and affects the selected segments would prevent regressions (and also
aligns with the PR description, which mentions this coverage).
--
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]
