Jackie-Jiang commented on code in PR #10239:
URL: https://github.com/apache/pinot/pull/10239#discussion_r1100594101
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentRestletResource.java:
##########
@@ -552,18 +552,21 @@ public SuccessResponse resetSegment(
@Produces(MediaType.APPLICATION_JSON)
@Authenticate(AccessType.UPDATE)
@ApiOperation(
- value = "Resets all segments of the table, by first disabling them,
waiting for external view to stabilize, and"
- + " finally enabling the segments", notes = "Resets a segment by
disabling and then enabling a segment")
+ value = "Resets all segments (when errorEVSegmentsOnly = false) or
segments with Error external view (when "
+ + "errorEVSegmentsOnly = true) of the table, by first disabling
them, waiting for external view to stabilize,"
+ + " and finally enabling them", notes = "Resets segments by
disabling and then enabling them")
public SuccessResponse resetAllSegments(
@ApiParam(value = "Name of the table with type", required = true)
@PathParam("tableNameWithType")
String tableNameWithType,
@ApiParam(value = "Name of the target instance to reset")
@QueryParam("targetInstance") @Nullable
- String targetInstance) {
+ String targetInstance,
+ @ApiParam(value = "Whether to reset only segments with error external
view") @QueryParam("errorEVSegmentsOnly")
+ @DefaultValue("false") boolean errorEVSegmentsOnly) {
Review Comment:
(optional) Suggest renaming it to `errorSegmentsOnly` since it is implicit
that ERROR is in EV, same for javadoc and variable names
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java:
##########
@@ -2447,21 +2446,34 @@ public void resetAllSegments(String tableNameWithType,
@Nullable String targetIn
Set<String> instanceSet = parseInstanceSet(idealState, segmentName,
targetInstance);
Map<String, String> externalViewStateMap =
externalView.getStateMap(segmentName);
for (String instance : instanceSet) {
- if (externalViewStateMap == null ||
SegmentStateModel.OFFLINE.equals(externalViewStateMap.get(instance))) {
- instanceToSkippedSegmentsMap.computeIfAbsent(instance, i -> new
HashSet<>()).add(segmentName);
+ if (errorEVSegmentsOnly) {
+ if (externalViewStateMap != null &&
SegmentStateModel.ERROR.equals(externalViewStateMap.get(instance))) {
+ instanceToResetSegmentsMap.computeIfAbsent(instance, i -> new
HashSet<>()).add(segmentName);
+ }
} else {
- instanceToResetSegmentsMap.computeIfAbsent(instance, i -> new
HashSet<>()).add(segmentName);
+ if (externalViewStateMap == null ||
SegmentStateModel.OFFLINE.equals(externalViewStateMap.get(instance))) {
+ instanceToSkippedSegmentsMap.computeIfAbsent(instance, i -> new
HashSet<>()).add(segmentName);
+ } else {
+ instanceToResetSegmentsMap.computeIfAbsent(instance, i -> new
HashSet<>()).add(segmentName);
+ }
}
}
}
- LOGGER.info("Resetting all segments of table: {}", tableNameWithType);
+ if (instanceToResetSegmentsMap.isEmpty()) {
+ LOGGER.info("No segments to reset for table: {}", tableNameWithType);
+ return;
+ }
+
+ LOGGER.info("Resetting segments of table: {}", tableNameWithType);
Review Comment:
Let's log all the segments to be reset
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java:
##########
@@ -2447,21 +2446,34 @@ public void resetAllSegments(String tableNameWithType,
@Nullable String targetIn
Set<String> instanceSet = parseInstanceSet(idealState, segmentName,
targetInstance);
Map<String, String> externalViewStateMap =
externalView.getStateMap(segmentName);
for (String instance : instanceSet) {
- if (externalViewStateMap == null ||
SegmentStateModel.OFFLINE.equals(externalViewStateMap.get(instance))) {
- instanceToSkippedSegmentsMap.computeIfAbsent(instance, i -> new
HashSet<>()).add(segmentName);
+ if (errorEVSegmentsOnly) {
+ if (externalViewStateMap != null &&
SegmentStateModel.ERROR.equals(externalViewStateMap.get(instance))) {
+ instanceToResetSegmentsMap.computeIfAbsent(instance, i -> new
HashSet<>()).add(segmentName);
+ }
} else {
- instanceToResetSegmentsMap.computeIfAbsent(instance, i -> new
HashSet<>()).add(segmentName);
+ if (externalViewStateMap == null ||
SegmentStateModel.OFFLINE.equals(externalViewStateMap.get(instance))) {
+ instanceToSkippedSegmentsMap.computeIfAbsent(instance, i -> new
HashSet<>()).add(segmentName);
+ } else {
+ instanceToResetSegmentsMap.computeIfAbsent(instance, i -> new
HashSet<>()).add(segmentName);
+ }
}
}
}
- LOGGER.info("Resetting all segments of table: {}", tableNameWithType);
+ if (instanceToResetSegmentsMap.isEmpty()) {
+ LOGGER.info("No segments to reset for table: {}", tableNameWithType);
+ return;
+ }
+
+ LOGGER.info("Resetting segments of table: {}", tableNameWithType);
for (Map.Entry<String, Set<String>> entry :
instanceToResetSegmentsMap.entrySet()) {
resetPartitionAllState(entry.getKey(), tableNameWithType,
entry.getValue());
}
- LOGGER.info("Reset segments for table {} finished. With the following
segments skipped: {}", tableNameWithType,
- instanceToSkippedSegmentsMap);
+ if (!instanceToSkippedSegmentsMap.isEmpty()) {
Review Comment:
Don't add this if check because we want to log when reset is done
--
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]
