SEZ9 commented on PR #11012: URL: https://github.com/apache/seatunnel/pull/11012#issuecomment-5770315219
@lazyfetch thanks for the ping, and sorry for the wait. Here is how I would suggest moving forward. **On the hanging `JdbcSybaseIT` job:** the problem is not only that the `nguoianphu/docker-sybase` image currently crashes on startup, but that the test has no bounded startup wait, so an image failure stalls the shared shard for hours instead of failing within minutes. So rather than only disabling the test: 1. Pin the image to a specific tag/digest and add a bounded startup timeout (a wait strategy with a hard limit of a few minutes) so a broken image fails fast with a clear error. 2. If the pinned image still cannot boot in the current CI environment, temporarily disabling/skipping the test with a comment pointing to this thread and the `docker-sybase` startup crash is the lower-risk path, so it can be re-enabled once a working image is available. Please note that I only have read-only review permissions here, so whether merging with the IT disabled is acceptable is a maintainer-level call that I can't sign off on alone. 3. If you find an alternative Sybase ASE image that boots reliably, that is the best outcome. I can't recommend a specific one, so please note which one you pick and why in the PR. **Still open from the earlier review** (please address or reply to each): - Type mapping: BINARY/VARBINARY/IMAGE, UNICHAR/UNIVARCHAR/UNITEXT, unsigned integers, BIGDATETIME/BIGTIME and rowversion TIMESTAMP are missing. Either map them or document them as unsupported. - Docs mention `table_path` / `table_list` / `use_regex`, but there is no Sybase Catalog in this PR. Please either state clearly that these rely on the driver metadata fallback, or drop them from the docs for now. - jTDS is LGPL (ASF Category X): it must stay provided/test scope and must not end up in the connector jar or the dist. Please confirm the pom reflects that. - `connection_check_timeout_sec`: jTDS does not implement `Connection.isValid()`, so validation must not depend on it (or the doc should say the option is not supported for Sybase). Changing the shared connection provider is out of scope here; Sybase-specific or doc-level handling is what I'm asking for. - Docs: add a short note on TLS/encrypted connections, since the examples are all plaintext TDS. - Docs location: move the Sybase page into the standard connector-v2 source docs directory so the ChangeLog import and sidebar work. Once you push the fail-fast change (and the disable, if needed) on top of `bca8da5` together with the items above, ping me and I'll re-review promptly. <!-- streview-comment:1221 --> -- 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]
