featzhang commented on PR #27567:
URL: https://github.com/apache/flink/pull/27567#issuecomment-4293213324
Thank you for the detailed review! Let me address each comment:
**davidradl's rename suggestion**: Already done - the method is named
`isRequestAllowed()` (line 218 of TritonCircuitBreaker.java).
**dianfu's counter dilution concern**: Already addressed - the code
implements a decay mechanism (lines 491-496) that halves counters when reaching
MAX_CLOSED_STATE_REQUESTS, preserving the relative failure rate while
preventing historical successes from diluting current failures.
**dianfu's allowHalfOpenRequest logic suggestion**: The current
implementation (lines 293-307) already matches the suggested behavior - it
throws TritonCircuitBreakerOpenException when `current >= halfOpenMaxRequests`,
which is exactly what your suggested code does.
**dianfu's failureThreshold validation**: Already implemented (lines
175-178) with validation ensuring the threshold is in (0.0, 1.0].
**dianfu's unused configurations/methods comment**: All the configurations
in TritonOptions are used by the health checker and circuit breaker. The getter
methods in TritonHealthChecker (isHealthy, getLastCheckTime, etc.) are part of
the public monitoring API and are intentionally provided for external
monitoring dashboards.
**dianfu's parameterized logging comment**: All logging in both files
already uses SLF4J's parameterized logging style (e.g., `LOG.info("...",
param1, param2)`), which is the recommended approach.
The code appears to already address all the concerns raised. Please let me
know if there are specific lines that need changes!
--
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]
