DanielLeens commented on PR #11814:
URL: https://github.com/apache/seatunnel/pull/11814#issuecomment-5852841665
Re: the "cut off" note — that comment wasn't actually truncated; it ends
"...no `1.8.21` entry in either file." as a complete sentence. No repost needed
either way, but wanted to clear that up in case it looked odd on your end.
I did go check F5/F7/F8 directly against `ec6a7f9affc` rather than leaving
this as "take your time" — here's what's actually there:
**F7 (redirects).** Resolved, and more solidly than I'd given credit for.
`createHttpClient()` sets both `.followRedirects(false)` and
`.followSslRedirects(false)` (`JobEventHttpReportHandler.java:283-284`), so
okhttp3 will never follow a redirect on this client at all — there's no
header-forwarding-on-redirect concern because redirects are never followed,
full stop. This is also pinned by a dedicated test,
`testDoesNotFollowRedirects` (`JobEventHttpReportHandlerTest.java:237-271`): it
enqueues three 307 responses with a `Location` header pointing at a second
`MockWebServer`, sends one report, asserts the original server received the
request with the configured `Authorization` header intact, and asserts the
redirect-target server's `takeRequest(1, TimeUnit.SECONDS)` is null (line
266-267) — i.e. zero requests reached the redirect target.
**F5 (retry test).** `testRetryAfterHttpFailure`
(`JobEventHttpReportHandlerTest.java:206-235`) doesn't wait on the
`Duration.ofDays(1)` scheduler interval at all — that duration only governs the
background scheduled cadence, not manual invocation. The test calls
`handler.report()` twice explicitly back-to-back (lines 220-221): the first
call hits the enqueued 500 and leaves the event in the local buffer (nothing
drained on failure), the second call re-sends the same buffered event and hits
the enqueued 200. `takeRequest` is called with an explicit 10-second timeout
both times (lines 223-224), and both response bodies are read inside a
try-with-resources over `Buffer` (lines 227-229), so they're closed either way.
**F8 (Content-Type).** This one is a real, verifiable difference, not just a
wording question. `httpMediaType = MediaType.parse("application/json")` carries
no charset, and `postEvents` calls the String-overload
`RequestBody.create(events, httpMediaType)` (`JobEventHttpReportHandler.java`
around the `postEvents` method). OkHttp3's static factory for that overload
auto-appends a charset when the caller's `MediaType` doesn't specify one and
the body is a `String` — so the header actually sent is `application/json;
charset=utf-8`, not bare `application/json`. This isn't a typo or hardcoded
literal in the handler; it falls out of using the String-based
`RequestBody.create` factory. It's also explicitly locked in by the test at
`JobEventHttpReportHandlerTest.java:190`, which asserts `"application/json;
charset=utf-8"` on the recorded request. So: the header is not unchanged — any
downstream collector doing exact-string matching on `Content-Type` rather than
parsing it would see a d
ifferent value than a bare `application/json`. Most JSON receivers won't care,
but I'd call this worth a one-line callout to @goutamadwant to confirm it's
acceptable (or trivial to pin the media type with an explicit charset if they'd
rather keep the string identical), rather than something I'd block on myself.
With F1/F2/F3/F4/F6 already closed in earlier rounds and F5/F7/F8 now
checked directly against source with test evidence, from my side there's
nothing left blocking on the code itself — the outstanding item remains the
`engine-v2-it` CI run actually reaching the console E2E module on this head, as
I noted in my last comment.
--
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]
