davidradl commented on PR #54:
URL:
https://github.com/apache/flink-connector-http/pull/54#issuecomment-5543715065
Just put this into my AI and it came back with:
1. HttpSinkClientWithRetry does not retry on network exceptions
In HttpSinkClientWithRetry.send(), the retry loop is wired only via
retryOnResult — it retries when hasRetryableRequests() returns true. However,
if the Java HTTP client throws (e.g., IOException for a connection timeout),
the CompletionStage completes exceptionally. Resilience4j's
decorateCompletionStage with only retryOnResult will not retry on exceptions
unless retryOnException is also configured.
Yet in HttpSinkResponseClassifier.classify(), null response →
RETRYABLE_FAILURE — the intent is clearly that network failures should be
retried. But that path is only reached when optResponse.orElse(null) is null
inside prepareAttemptResult, which only runs if submitRequests succeeds. If the
future itself fails, the path never reaches the classifier.
Suggested fix: Add .retryOnException(IOException.class::isInstance) (or
similar) to the RetryConfig built in HttpSinkClientWithRetry, and add a test
case for it.
2. Default SINK_MAX_RETRIES = 3 changes behaviour for existing deployments
The option has a hard default of 3 in HttpDynamicSinkConnectorOptions.
Previously retries were not performed at all. Any existing deployment that
doesn't set this option will silently start retrying on 500/503/504,
potentially tripling the number of HTTP calls on degraded endpoints. This is a
silent behaviour change for existing users.
This should either default to 0 (explicit opt-in), or be clearly called out
in the PR description / NOTICE / migration guide, and the compatibility note in
the PR body updated.
3. SinkRetryConfigProvider will throw at runtime if maxRetries == 0
return createBuilder().maxAttempts(sinkConfig.getMaxRetries() + 1).build();
When maxRetries = 0, maxAttempts = 1 — that's valid, Resilience4j will make
exactly one attempt. But createBuilder() also unconditionally reads
getRetryStrategy() and tries to resolve RetryStrategyType.fromCode(...). If
strategy config is missing and retries are disabled, this still constructs and
validates the full retry configuration. Consider short-circuiting when
maxRetries == 0 to avoid spurious validation errors and unnecessary overhead.
4. IGNORED responses are added to successfulRequests in the accumulator
In ResponseAccumulator.add(), IGNORED entries flow into successfulRequests.
They're excluded from fatalFailedRequests and failedRequests, so the retry
accumulator never sees them — but then
SinkHttpClientResponse.successfulRequests conflates "succeeded" with "ignored",
which may mislead callers who inspect getSuccessfulRequests() for post-callback
processing. Consider a dedicated ignoredRequests field on
SinkHttpClientResponse, even if the writer just combines them. This would also
align the response shape with the four-way HttpSinkResponseStatus enum.
5. legacyResponseChecker short-circuits before the new ignoredResponseCodes
check
In HttpSinkResponseClassifier.classify():
if (ignoredResponseCodes.contains(response.statusCode())) {
return HttpSinkResponseStatus.IGNORED; // ✓ checked first
}
if (legacyResponseChecker != null) { // ← then legacy takes over
entirely
If a user is on the legacy path and sets sink.ignored-response-codes, the
ignored check wins — which is the correct precedence. But if a user is on the
legacy path and does NOT set ignored codes, the new responseChecker is never
consulted. That's intentional for compatibility, but it means
sink.success-codes and sink.retry-codes are silently ignored when legacy
error-code properties are present. This should be documented or at least logged
as a warning.
6. DefaultSinkHttpClientContext is not Serializable
HttpSinkInternal.DefaultSinkHttpClientContext implements
SinkHttpClientContext but is not serializable. The context is created
transiently in buildSinkHttpClient(), so this is safe today. But
SinkHttpClientContext itself is @PublicEvolving and users implementing custom
SinkHttpClientBuilder will receive it — if they ever try to capture it across a
serialisation boundary, they'll get a NotSerializableException at runtime.
Consider either marking SinkHttpClientContext with extends Serializable or
adding a Javadoc warning.
7. HttpSinkConfig has a redundant getReadableConfig() method
HttpSinkConfig stores a Configuration field (named readableConfig) and adds
a method getReadableConfig() returning ReadableConfig. Lombok's @Getter would
already generate getReadableConfig() returning Configuration. The custom
override returning the ReadableConfig supertype while the @Getter-generated
field is Configuration will cause a compilation conflict. This should be
verified — it may require suppressing the Lombok getter for that field
specifically.
8. HttpSinkBuilder.resolveSinkConfig() will NPE if endpointUrl is null
HttpSinkConfigFactory.fromDataStream(endpointUrl, ...) is called eagerly in
build() before the existing null/blank check on endpointUrl that was in
HttpSinkInternal. That check now operates on sinkConfig.getUrl() inside
HttpSinkInternal, so the validation still happens — but it happens after
resolveSinkConfig() already ran. If endpointUrl is null, fromDataStream passes
it into the config builder without validation. The Preconditions check should
move to resolveSinkConfig() or HttpSinkBuilder.build().
9. Deprecated SinkHttpClientResponse constructor uses
HttpRequest.getRequestEntries()
The new compatibility constructor flattens List<HttpRequest> via
request.getRequestEntries(). HttpRequest previously had no requestEntries
field; this PR adds it as a constructor parameter. However, old custom clients
that construct HttpRequest using the old three-arg constructor will get a
NoSuchMethodError at runtime — the deprecated constructor doesn't actually
provide binary compatibility. This should be noted in the Javadoc or the field
given a nullable default path.
10. The SINK_WRITER_THREAD_POOL_SIZE default changed from "4" (string) to 1
HttpSinkWriter previously defaulted to 4 threads
(HTTP_SINK_WRITER_THREAD_POOL_SIZE = "4").
HttpDynamicSinkConnectorOptions.SINK_WRITER_THREAD_POOL_SIZE now has
.defaultValue(1). This is a silent performance regression for existing users.
Intentional?
--
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]
