bharathgunapati commented on PR #54: URL: https://github.com/apache/flink-connector-http/pull/54#issuecomment-5560030997
Thanks for running this through, @davidradl — really useful list. Pushed `b085416` addressing it. Point by point: **1. Retry on network exceptions.** Exception retries are now explicitly scoped to `IOException` (unwrapping `CompletionException`) in `HttpSinkClientWithRetry`, matching `HttpClientWithRetry` on the lookup side. One correction to the analysis: Resilience4j's default exception predicate is `throwable -> true`, so exceptions were already being retried — the change makes the intent explicit and, more importantly, stops non-retryable errors (e.g. a user callback throwing) from re-submitting the whole batch. Tests added in `HttpSinkClientWithRetryTest` and `JavaNetSinkHttpClientTest`. **2. Default `max-retries`.** Agreed, this was a silent behaviour change. `http.sink.max-retries` now defaults to `0`, so existing jobs keep send-once behaviour and retries are opt-in. The connection tests that relied on a retry now set it explicitly. **3. `maxRetries == 0` still building a full retry config.** Fixed — `SinkRetryConfigProvider` now short-circuits to `RetryConfig.custom().maxAttempts(1)` and never resolves the retry strategy when retries are disabled. Since this is now the default path, there's a test covering an invalid strategy value with retries off. **4. IGNORED lumped into `successfulRequests`.** `SinkHttpClientResponse` already changes shape here (`fatalFailedRequests` is new), so doing the ignored split now means consumers adapt once instead of twice. Ignored responses land in `getIgnoredRequests()`; the writer still treats them as committed and non-retryable. **5. Legacy error-code path ignoring the new options.** Kept legacy precedence for compatibility, but it is no longer silent: `HttpSinkResponseClassifier` logs a WARN naming both legacy properties and the `http.sink.success-codes` / `http.sink.retry-codes` options being ignored. Also called out on the option descriptions and in the table docs (en + zh). **6. `DefaultSinkHttpClientContext` not `Serializable`.** Documented on `SinkHttpClientContext` that instances are created at writer open time and are not part of checkpoint or job-graph serialization, so they must not be retained or serialized. I did not add `Serializable`, since the object never crosses that boundary and it would widen the public API for a runtime-only type. **7. Lombok `getReadableConfig()` conflict.** Not reproducible — Lombok's `@Getter` skips generation when a method of that name already exists. `javap` on `HttpSinkConfig` shows a single `getReadableConfig()` returning `ReadableConfig`. This is the same pattern as `HttpLookupConfig` on `main`. **8. NPE if `endpointUrl` is null.** A null URL does not NPE: `HttpSinkConfigFactory` passes it through and `HttpSinkInternal` rejects it during `build()` with `IllegalArgumentException: The endpoint URL must be set when initializing HTTP Sink.` `HttpSinkBuilderTest#testNullUrl` already covers this and passes. **9. Deprecated `SinkHttpClientResponse` constructor.** Expanded the `@deprecated` javadoc. For the record, the constructor itself *is* binary compatible — generics erase, so the descriptor is `(Ljava/util/List;Ljava/util/List;)V` in both versions. The real caveat is on the read side: `getSuccessfulRequests()` / `getFailedRequests()` keep the same erased descriptor but now return `HttpSinkRequestEntry` elements, so old compiled readers would link and then hit a `ClassCastException`. That is now stated explicitly in the javadoc. **10. Writer thread pool default 4 → 1.** Keeping `1`, since that is what the docs have always documented. Now called out explicitly in the option description and in both datastream and table docs: older versions used 4 threads when the option was unset, and users can set the option to `4` to keep the previous throughput. On the naming point from earlier: the user-facing wording is "status-code handling" throughout the PR and docs. I've kept `HttpSinkResponseClassifier` as an internal name because it maps a response to the sink's outcome (`SUCCESS` / `IGNORED` / `RETRYABLE_FAILURE` / `FATAL_FAILURE`) rather than describing a configurable option. Happy to rename if you'd still prefer it gone. Full module test suite is green: 631 tests, 0 failures. -- 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]
