xiangfu0 commented on code in PR #20018:
URL: https://github.com/apache/pinot/pull/20018#discussion_r4365305325
##########
pinot-common/src/main/java/org/apache/pinot/common/metrics/ControllerMeter.java:
##########
@@ -59,6 +59,8 @@ public enum ControllerMeter implements AbstractMetrics.Meter {
LLC_SEGMENTS_DEEP_STORE_UPLOAD_RETRY_SUCCESS("LLCSegmentDeepStoreUploadRetrySuccess",
false),
LLC_SEGMENTS_DEEP_STORE_UPLOAD_RETRY_ERROR("LLCSegmentDeepStoreUploadRetryError",
false),
SEGMENT_MISSING_DEEP_STORE_LINK("RealtimeSegmentMissingDeepStoreLink",
false),
+ // Number of failed repair attempts (segment reset or re-ingestion trigger)
in the periodic error-segment repair
Review Comment:
Done: the meter comment now spells out the contract: it counts repairs the
pass failed to initiate (a reset that could not be sent, or a re-ingestion
trigger that failed), once per affected segment per pass, so a segment that
fails again on a later pass is counted again; repairs skipped by policy and
repairs that fail asynchronously after they were sent are not counted. The
Javadoc of repairSegmentsInErrorState says the same.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManagerTest.java:
##########
@@ -3032,6 +3032,43 @@ public void
testRepairSegmentsInErrorStateCountsFailedRepairsAsUnrecoverable()
assertTrue(exception.getMessage().contains(donePartialErrorSegment),
exception.getMessage());
verify(mockControllerMetrics).setOrUpdateTableGauge(REALTIME_TABLE_NAME,
ControllerGauge.PAUSELESS_SEGMENTS_IN_UNRECOVERABLE_ERROR_COUNT, 2L);
+ // The failed re-ingestion trigger and the failed reset are each metered
+ verify(mockControllerMetrics,
times(2)).addMeteredTableValue(REALTIME_TABLE_NAME,
+ ControllerMeter.LLC_ERROR_SEGMENT_REPAIR_FAILURES, 1L);
+ }
+
+ @Test
+ public void
testRepairSegmentsInErrorStateMetersFailedResetsOnNonPauselessTable() {
Review Comment:
Done: testRepairSegmentsInErrorStateMetersFailedResetsOnNonPauselessTable
now resets a three-segment batch with two failed segments and asserts a metered
value of 2L, so replacing segmentsFailedToReset.size() with 1 would fail it.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
--
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]
