DanielLeens commented on PR #12858:
URL: https://github.com/apache/seatunnel/pull/12858#issuecomment-6567766468
Thanks @SEZ9 for the careful pass, and @karthik-0306 for the detailed
mapping.
One practical note first: @karthik-0306's status comment is not actually cut
off on GitHub. The F3 line ends "...across snapshot, select, run and restore",
and F4 to F8 follow in full. It looks like the text you saw was truncated by
the preview. To save a round trip, here are the items you asked about, checked
against head `5e82f43` in
`tools/seatunnel-python-sdk/seatunnel/plugin_installer.py`:
- **F3 (lock release on success and failure)**: `_exclusive_lock` (L207-227)
opens the lock file, takes `flock(LOCK_EX)` and `yield`s inside a `try`; the
`finally` calls `os.close(fd)`, which releases the lock on a normal return, on
`PluginInstallError`, and on `KeyboardInterrupt`. `install_plugins` holds it
(L301) around read-snapshot, `select_plugins`, `subprocess.run` and the
rollback, so the restore also happens under the lock. The lock file is
deliberately left on disk, and the docstring explains why.
`test_lock_is_held_while_the_script_runs` covers the held-while-running side. I
did not find a test that asserts the lock is released after a failure; that is
guaranteed structurally by the `finally`, so I see it as optional.
- **F4 (Windows)**: it is an explicit unsupported-platform error, not
`install-plugin.cmd`. `_is_windows()` is checked first in `install_plugins`
(L263), before the version check and before `shutil.which("bash")` (L297), so a
WSL or Git `bash.exe` is never discovered or handed a Windows path. The
docstring says why `.cmd` is not used: it does not report download failures
through its exit code, which would defeat the rollback. `select_plugins()`
still works on any platform. Test: `test_windows_is_rejected_before_rewriting`.
- **F5 (version)**: empty or non-string input is rejected (L269-271), then
the value must fully match `[A-Za-z0-9._-]` (L274), the same character set
`install-plugin.sh` accepts, so `[`, `,`, `+`, spaces and shell metacharacters
are refused before anything is rewritten. Tests: `test_empty_version_raises`,
`test_invalid_version_raises_before_rewriting`.
- **F6**: non-UTF-8 content and `OSError` on read, a non-iterable
`connectors`, and non-string connector or version values all surface as
`PluginInstallError`. Tests: `test_non_utf8_file_raises_plugin_install_error`,
`test_invalid_connectors_argument_raises_plugin_install_error`.
- **F7**: rollback goes through `_restore_if_changed` (L191-199, called at
L328), which writes only when the current bytes differ from the snapshot. Test:
`test_failed_install_does_not_rewrite_an_unchanged_file`.
- **F8**: the error hints no longer name a concrete version; they say it
should match the SeaTunnel engine version. `test_empty_version_raises` asserts
"engine version" is present and "3.0.1" is absent.
This is a static read of the code and the author's test list; I did not
execute anything locally. My earlier approval still stands for this unchanged
head, and the exact-head fork Build was successful when I last checked. A
write-capable maintainer still needs to give the final approval for the merge
gate.
--
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]
