davidradl commented on PR #54:
URL:
https://github.com/apache/flink-connector-http/pull/54#issuecomment-5635203620
another AI review
1. HttpSinkClientWithRetry creates a new Retry instance on every send() call
In HttpSinkClientWithRetry.send():
Retry retry = Retry.of("http-sink-connector",
RetryConfig.<HttpSinkAttemptResult>from(retryConfig)…);
Retry.of() registers into a global Resilience4j registry under that name. If
the name already exists (another sink, a test that ran before), it silently
returns the first registered config, not the one you just built. This is a
correctness risk in multi-sink or test JVMs. Fix: use a random name per
instance, or build the Retry once in the constructor and reuse it (the
requestEntriesToSubmit AtomicReference already handles per-call state).
2. Behaviour change: fatal failures now throw instead of silently completing
Before this PR, both the "no response" and "error status" branches called
resultHandler.complete() (silent drop). After the PR, all three bad branches
call resultHandler.completeExceptionally(...). This is a correctness
improvement, but it's also a breaking behaviour change for any existing job
that relied on silent-drop semantics. The compatibility note in the PR
description mentions API changes but doesn't mention this operational change.
Existing jobs using http.sink.error.code to mark 5xx as errors will now see
those jobs fail unless the codes are mapped to an ignored set.
3. Verify ResponseAccumulator captures the final successful attempt
In HttpSinkClientWithRetry, responseAccumulator.add() is called inside the
retryOnResult predicate. The thenApply callback only calls markRetriesExhausted
when hasRetryableRequests() is true. If the final attempt succeeds (no
retryable requests), the question is whether Resilience4j's retryOnResult
callback still fires for that last attempt before stopping. If it does not, the
last batch of successful requests is silently missing from the accumulated
response. Worth adding an explicit test for a "succeeds on the Nth retry" path
and asserting the full successful count.
4. ignoredResponseCodes is evaluated before the legacy checker, yet the log
warning says legacy props disable the new options
In HttpSinkResponseClassifier.classify():
if (ignoredResponseCodes.contains(response.statusCode())) {
return IGNORED; // evaluated even in legacy mode
}
if (legacyResponseChecker != null) { // legacy path
…
}
When legacy properties are active, ignoredResponseCodes is populated from
http.sink.error.code.exclude (via setLegacyIgnoredResponseCodesIfNeeded). So
excluded codes are classified as IGNORED rather than as legacy-non-errors. This
is semantically correct, but the warning log says the new success-codes /
retry-codes options are fully ignored when legacy props are set — that is
slightly misleading. The Javadoc or warning should clarify that
ignoredResponseCodes continues to serve as the "excluded" set even in legacy
mode.
5. HttpSinkConfig.getReadableConfig() vs Lombok @Getter duplicate
HttpSinkConfig has @Getter on the class (from Lombok) and also a
hand-written getReadableConfig() with a narrowed ReadableConfig return type.
The Lombok-generated getReadableConfig() would return Configuration, so the
hand-written one hides it to return the interface. This is functional but
confusing — the field is still accessible as Configuration via the Lombok
getter on readableConfig. Consider either removing @Getter on readableConfig
and relying solely on the hand-written method, or dropping the hand-written
method if callers don't need the narrowed type.
6. HttpSinkConfigFactory DataStream path: manual property-to-config mapping
is fragile
HttpSinkConfigFactory.fromDataStream() has ~12 individual
getProperty()/set() blocks. Each new config key needs a corresponding block
here. A table-driven approach (iterating over ConfigOption definitions and
their HttpConnectorConfigConstants string counterparts) would be more
maintainable and less likely to miss a new option in a follow-up PR.
--
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]
