SiriusTL666 commented on PR #13009:
URL: https://github.com/apache/seatunnel/pull/13009#issuecomment-6434931666
A note on CI, since this PR will show `action_required` and no run:
The `pull_request` workflows are gated because this is still a
first-time-contributor PR, which is expected. What is unusual is that I cannot
produce the equivalent fork build this time: `POST
/repos/SiriusTL666/seatunnel/actions/workflows/{id}/dispatches` now returns
`422 Actions has been disabled for this repository`, and pushing the branch did
not create any `push` run either (the repo reports `total_count: 0` for runs on
this branch, and zero check suites on head `805b285f`). GitHub disabled Actions
on the fork itself; `PUT /repos/.../actions/permissions` returns `204` but the
state does not change. On the previous PR the same dispatch worked, so this is
a change on the fork side, not on this branch.
So the CI evidence for this head is local only. Everything I can check
locally, I did:
- JDK 17, module `connector-file-ftp`: **26/26 pass** — `FtpFileFactoryTest`
4, `SeaTunnelFTPFileSystemTest` 13 (the existing fake-FTP-server suite,
unmodified), new `SeaTunnelFTPFileSystemTimeoutTest` 9.
- `spotless:check` passes.
- Both fixes reverse-verified: reverting `setDefaultTimeout` turns 4 tests
red, reverting the two `disconnect` calls turns 2 red.
If approving the workflows is not possible right away, the local result
above is what I am relying on, and I am happy to re-run anything a committer
wants on their side. I am not asking for a merge on local evidence alone —
flagging it so the missing check does not read as an unaddressed failure.
---
On the change itself, the one thing worth a reviewer's attention is the
default. Bounding the control-channel read means a job against a
reachable-but-silent server now fails with a timeout where it previously hung
in `RUNNING` forever. That is the intended fix, but it is a behaviour change,
so it is recorded under `dev` in both `incompatible-changes.md` files, and
`control_timeout = 0` restores the old behaviour.
I deliberately did **not** touch the data-connection timeout.
`setDefaultTimeout` covers the control channel only; the data channel has a
separate `dataTimeout` whose default is "no timeout". Extending the change
there would alter how long a slow but healthy bulk transfer is allowed to
stall, which is a different decision from "do not hang forever on a reply that
never comes". If you think the data channel should be bounded too, I would
rather do it as a separate change with its own default discussion than fold it
in here.
--
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]
