featzhang commented on PR #33: URL: https://github.com/apache/flink-connector-http/pull/33#issuecomment-4161488480
## Code Review Fixes Thanks for the review feedback. I've addressed the issues found during code review and pushed the fixes in commit 2b9ecf6. ### Issues Fixed **🔴 Premature error counter increment (Critical)** The original code called `numRecordsSendErrorsCounter.inc()` at the start of the `err != null` branch, before determining whether a retry would be attempted. This caused error metrics to be incremented on every failure — even when the request would be retried and eventually succeed. **Fix:** Moved `numRecordsSendErrorsCounter.inc()` into the exhausted-retries branch only. Errors are now counted exactly once, after all retry attempts have been exhausted. **🟡 HTTP-level failures not retried** The original code handled `response.getFailedRequests().isEmpty() == false` by immediately recording an error and giving up — no retry was attempted for HTTP-level failures (e.g. 5xx responses), only for `IOException`. **Fix:** Added retry logic for HTTP-level failures, consistent with the `IOException` retry behavior. The original `requestEntries` are retried (rather than the internal `HttpRequest` objects, which are not accepted by `putRequests()`). **🟢 Code duplication** The retry scheduling logic (sleep + recursive call) was duplicated in two branches. **Fix:** Extracted a `scheduleRetry()` helper method to eliminate the duplication. --- ### New / Updated Tests | Test | Description | |------|-------------| | `testRetryOnHttpFailedRequests` | Verifies HTTP-level failures trigger retry; on eventual success, no error is counted and `requestResult.accept()` is called exactly once | | `testNoRetryWhenDisabled` | Verifies `retry.times=0` skips retries entirely; `accept()` is called exactly once | All 4 tests pass locally: ``` Tests run: 4, Failures: 0, Errors: 0, Skipped: 0 — BUILD SUCCESS ``` Please take another look when you get a chance. Thanks! -- 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]
