SEZ9 commented on PR #12384: URL: https://github.com/apache/seatunnel/pull/12384#issuecomment-5924307039
Thanks for the update at 6280e5ea — the earlier blockers from 967aab23 (test compilation, zero-timeout semantics, pin tests that actually fail on regression) are resolved, so this is in good shape. What remains are the follow-ups from my previous review; none are blocking on their own, but it would help to hear your preference on each: **Would be nice to address in this PR** - **Retry options unconstrained (F1):** `max_retries`, `retry_backoff_multiplier_ms` and `max_retry_backoff_ms` still accept negative values, which either silently drop data or surface as an uncaught `IllegalArgumentException` in `InfluxDBSinkWriter.flush()`. Since the goal of this PR is to reject bad values at validation time, please consider adding `greaterOrEqual(..., 0)` constraints for these three in `InfluxDBSinkFactory` (plus a rejection test). - **`batch_size > 0` vs. the writer's `batch_size = 0` path (F3):** the writer still explicitly implements "flush only on checkpoint/close" for `0`, so the `> 0` constraint rejects a mode the code supports. Either relax it to `>= 0`, or remove the writer guard and document that `0` is no longer valid — please pick one and note which in the PR description. - **`write_timeout` docs vs. the new test (F2, F4):** the test now asserts `write_timeout` is never read at runtime, but the en/zh sink docs still describe it as the client's write timeout. Please update the prose (and the option tables, which weren't updated alongside the per-option text) so the docs match what the test pins. **Fine as follow-ups, just confirm** - **Source side (F5):** source and sink share `InfluxDBClient.getInfluxDB`, so negative timeouts still fail at task start on the source. Happy to take this in a separate PR if you'd rather keep this one sink-only. - **Upper bound on timeouts (F6):** values above `Integer.MAX_VALUE` ms are rejected by OkHttp at writer construction rather than at validation. Low priority; a note is enough. - **Tests (F7, F8):** the constraint-rejection tests only assert the exception type — asserting the offending key appears in the message (as the required-option tests already do) would make them consistent. Also, `connect_timeout_ms` is fed `Long` literals, which skips the Integer→Long conversion real HOCON configs go through; one case using an `Integer` would cover that path. If you push F1/F3/F2+F4, I'll take a final look right away. <!-- streview-comment:1449 --> -- 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]
