DanielLeens commented on PR #11814:
URL: https://github.com/apache/seatunnel/pull/11814#issuecomment-5805501124
Confirmed independently, SEZ9 - I re-fetched the exact head (`ec6a7f9affc`)
and checked each commit rather than trusting the description. `8f4dce57d45`
("Attribute bundled Public Suffix List data") touches only
`seatunnel-dist/release-docs/LICENSE`, `NOTICE`, and the new
`licenses/LICENSE-publicsuffix.txt` - no code or test file. `af9b89d9e` and
`ec6a7f9affc` are both `dev`-sync merges; I listed every file each one touches
and none of them is `JobEventHttpReportHandler.java`, its test, either
`pom.xml`, or the console-E2E module this PR owns - all of it is unrelated dev
content (amazondocumentdb, cdc-postgres, mqtt, rocketmq, redis, tablestore,
fluss). So yes, no code or test changes to this PR's own files since
`73f66f922`, matching what my own `APPROVED` review at this exact head already
verified by direct diff.
Per-item status, re-checked against source at `ec6a7f9affc`:
- **F1 (E2E wait strategy):** Still configured. `executeExtraCommands`
(`FakeSourceToConsoleWithEventReportIT.java:94-126`) keeps the base fixture's
`LogMessageWaitStrategy.withRegEx(".*received new worker register:.*")` as the
actual readiness gate (line 125); the wrapper added around it only calls
`logStartupThreads()` on a failed wait to capture `jps`/`jstack` diagnostics
(lines 116-123, 128-156) - it does not change what gates readiness.
- **F2/F3 (`close()` robustness):** Unchanged from my last review. `close()`
unwraps `CompletionException` before the type check, logs the expected
`HazelcastInstanceNotActiveException` case at INFO without a stack trace, and
everything else at ERROR with the original exception; the local-buffer flush
still runs afterward regardless of which branch fired, and
`cancelAll()`/`evictAll()` plus interrupt-flag restoration run unconditionally
in `finally`.
- **F4/F6 (Kotlin stdlib graph):** `seatunnel-engine-server/pom.xml` imports
a scoped `kotlin-bom:1.9.10` and declares `kotlin-stdlib-jdk8` without a
version. `tools/dependencies/known-dependencies.txt` and
`seatunnel-dist/release-docs/LICENSE` both list exactly `kotlin-stdlib`,
`-common`, `-jdk7`, `-jdk8` at `1.9.10` - I grepped both files for `1.8.21` and
found nothing, so there is no remaining stale entry.
- **F5 (`testRetryAfterHttpFailure`):** Yes. It uses
`retryServer.takeRequest(10, TimeUnit.SECONDS)` (a bounded timeout, not an
unbounded wait), closes both request `Buffer`s via try-with-resources, and
drives the scheduler explicitly by calling `handler.report()` twice, even
though the handler is constructed with `Duration.ofDays(1)` - it is not relying
on the 1-day interval firing on its own.
- **F7 (redirects):** Disabled. `createHttpClient()` sets
`.followRedirects(false).followSslRedirects(false)`, and
`testDoesNotFollowRedirects` covers it directly.
- **F8 (Content-Type charset):** Byte-identical to previous behavior.
`MediaType.parse("application/json")` carries no charset parameter, so the
emitted header is exactly `application/json` - there is no `; charset=utf-8`
suffix, and this is not one of the two intentionally documented behavior
changes (only "no redirects" and "TLS 1.2/1.3 only" are).
Nothing here changes my prior `Ready to merge` conclusion; the only open
item remains CI evidence on this exact head, which was still not green as of my
last review.
--
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]
