You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "Jackie-Jiang (via GitHub)" <gi...@apache.org> on 2023/02/08 19:40:11 UTC
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #10239: update an existing API to reset only segments with Error EV
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org