CodeTrainerMan opened a new pull request, #1142: URL: https://github.com/apache/flink-agents/pull/1142
Linked issue: #1136 ### Purpose of change #### User-visible outcome `RetryExecutor.builder()` now rejects a misconfigured retry policy at build time and names the parameter. Previously a negative `maxRetries` made `execute()` throw `NullPointerException` without ever invoking the operation, and a negative `initialBackoffMs` could surface as `IllegalArgumentException: timeout value is negative` from `Thread.sleep`. #### Intent Both are reachable from user-supplied configuration. Fail fast where the value is supplied, with a message that names the parameter, instead of later at a point that hides the cause. #### Runtime flow `Builder.build()` runs three checks in order before constructing the executor: `maxRetries < 0`, `initialBackoffMs < 0`, `maxBackoffMs < initialBackoffMs`. Each raises `IllegalArgumentException` and skips construction, so no executor holding an invalid policy is ever created. `execute()` is unchanged; with `maxRetries >= 0` the loop body runs at least once, so `lastException` is always assigned before the final `RuntimeException`. #### Key decisions - Validate in `build()` rather than in each setter: a setter cannot compare `maxBackoffMs` against `initialBackoffMs`, since either may be supplied first. - `IllegalArgumentException`, matching how an invalid argument is rejected elsewhere in this code base. - Rejected: clamping negative values to the defaults. That hides the misconfiguration, which is the problem being fixed. - Given the first two checks pass, `maxBackoffMs < 0` is unreachable, so it is not checked separately. ### Behavioral Semantics #### Interaction decisions | maxRetries | initialBackoffMs | maxBackoffMs vs initial | build() | |---|---|---|---| | < 0 | any | any | IAE naming `maxRetries` | | >= 0 | < 0 | any | IAE naming `initialBackoffMs` | | >= 0 | >= 0 | smaller | IAE naming both backoff parameters | | >= 0 | >= 0 | >= | constructs, no other effect | #### Behavioral contracts 1. A negative `maxRetries` raises `IllegalArgumentException` whose message names `maxRetries` and echoes the value. 2. A negative `initialBackoffMs` raises `IllegalArgumentException` whose message names `initialBackoffMs` and echoes the value. 3. `maxBackoffMs < initialBackoffMs` raises `IllegalArgumentException` naming both parameters. 4. `maxRetries == 0` constructs, and `execute()` invokes the operation exactly once and reports failure through a `RuntimeException` carrying the cause. 5. `withDefaults()` and every previously legal configuration still construct unchanged. #### Failure behavior Each invalid configuration raises `IllegalArgumentException` from `build()`; nothing is clamped, logged, or absorbed, and no executor is created. The failure paths of `execute()` - non-retryable exception, retries exhausted, interrupt - are untouched. The `NullPointerException` a negative `maxRetries` used to cause is no longer reachable, because no such executor can be built. ### Tests `mvn -pl api -am -Dtest=RetryExecutorTest test` -> 15 tests, 0 failures: 11 pre-existing and unchanged, 4 new. `mvn -pl api spotless:check` passes. | Contract | Test | |---|---| | 1 | `RetryExecutorTest.testBuildRejectsNegativeMaxRetries` | | 2 | `RetryExecutorTest.testBuildRejectsNegativeInitialBackoffMs` | | 3 | `RetryExecutorTest.testBuildRejectsMaxBackoffBelowInitialBackoff` | | 4 | `RetryExecutorTest.testBuildAcceptsBoundaryValues` | | 5 | `RetryExecutorTest.testDefaultConfiguration` (pre-existing) | Coverage by risk: three guards, each pinned by a test asserting both the exception type and the message, plus one boundary test that also drives `maxRetries == 0` through `execute()`. Not verified: the `window * 2` growth at `Long.MAX_VALUE`, pre-existing and untouched; the interrupt path keeps only its pre-existing test. <details> <summary>Implementation invariants and supporting evidence</summary> - Check order is fixed: `maxRetries`, then `initialBackoffMs`, then the backoff comparison. A caller supplying several invalid values sees the first one only. - With `initialBackoffMs >= 0` and `maxBackoffMs >= initialBackoffMs`, `maxBackoffMs >= 0` follows, so a separate `maxBackoffMs < 0` check is redundant. - No call site in the repository passes a negative value, checked across the Java sources, so no caller needs updating. </details> ### API No signature is added, removed, or changed. `Builder.build()` now throws `IllegalArgumentException` for three configurations that previously constructed; a caller passing one of them was already broken and now fails at the point of configuration with the parameter named. A caller using legal values, including `withDefaults()` and `maxRetries(0)`, is unaffected. ### Documentation - [ ] `doc-needed` - [x] `doc-not-needed` - [ ] `doc-included` ### Was this patch authored or co-authored using generative AI tooling? - [x] Yes - [ ] No Generated-by: CodeBuddy (Auto) -- 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]
