[Feature][Client] Add install-plugin helper to Python SDK by karthik-0306 · Pull Request #12858 · apache/seatunnel

karthik-0306 · 2026-10-18T07:07:11Z

Purpose of this pull request

Related to #12707 (pip-installable SeaTunnel launcher).

This PR contributes the install-plugin piece of the launcher core that @SEZ9 outlined on #12707 to Apache SeaTunnel. It adds tools/seatunnel-python-sdk/seatunnel/plugin_installer.py with two functions:

select_plugins(plugin_config_path, connectors) rewrites config/plugin_config so that only the requested connectors are enabled. It accepts short names (jdbc) or artifact IDs (connector-jdbc).
install_plugins(seatunnel_home, connectors, version) checks the platform, the version, the distribution layout and that bash is available, then calls select_plugins and runs bin/install-plugin.sh <version> under an exclusive lock. If the script fails, plugin_config is restored to its exact content from before the call.

Behaviour:

Lines are classified the same way install-plugin.sh reads them: blank lines and lines starting with - (section headers) or # (comments) are skipped, and every other line is a connector. Whitespace around a connector ID is tolerated (#connector-kafka is recognized as disabled). Section headers, the license header, comments, blank lines and line endings (LF or CRLF) are preserved.
Idempotent: a second run with the same connectors leaves the file byte-identical, and the file is not rewritten.
If a requested connector is not listed, the request is empty or invalid (including None or non-string names), the file contains a line install-plugin.sh would reject (for example connector-kafka # keep), or the file is not valid UTF-8, it raises PluginInstallError and does not modify the file.
Everything that can fail before the script starts is checked before plugin_config is touched: the platform, an empty or invalid version (validated with the script's own character set), an invalid seatunnel_home, a missing bin/install-plugin.sh or config/plugin_config, and a missing bash. A relative seatunnel_home is resolved to an absolute path, so bash always gets an absolute script path.
Rollback on failure: the original bytes of plugin_config are saved before the selection. If install-plugin.sh exits non-zero, cannot be started (OSError), or is interrupted (Ctrl+C during a download), they are written back, but only if the file actually changed, so a failed install leaves no changed selection. The selection itself runs inside the rollback guard, so an interrupt right after the rewrite is rolled back too. The original error is still raised. If the restore itself fails, a regular error becomes one PluginInstallError naming both failures, while KeyboardInterrupt / SystemExit are re-raised as they are (so the caller still stops), with the restore failure attached as __context__ (and as a note on Python 3.11+).
Concurrency: the whole snapshot → select → run → restore sequence holds an exclusive fcntl.flock on config/.plugin_config.lock, so a concurrent call on the same copy waits instead of installing another call's selection.
Platform: install_plugins() is supported on Linux and macOS only, and raises a clear error on Windows before touching anything. bin/install-plugin.cmd never checks errorlevel or exits non-zero, so a failed download would look like success and the rollback could not work. select_plugins() itself works on any platform.
The file is written through a temporary file plus os.replace, with its file mode kept, so a failure never leaves it half written.
The script's output is not captured, so download progress is shown. SEATUNNEL_PLUGIN_DOWNLOAD_METHOD and SEATUNNEL_MAVEN_REPOSITORY are passed through unchanged. version is required and has no default, matching the issue's "wheel version equals engine version" rule; LATEST, RELEASE and *-SNAPSHOT make the script download through Maven, as documented.

Ownership boundary: select_plugins() and install_plugins() rewrite config/plugin_config in place. They must only be given a SeaTunnel copy that the caller owns and manages, such as the launcher's per-user / per-version cache directory, never a source checkout or a shared installation, where the connector selection would change for everyone using it. This is stated in the module docstring and in the seatunnel_home / plugin_config_path parameter docs. CRLF files are preserved, but install-plugin.sh itself only accepts LF endings, which the select_plugins() docstring notes.

Design note: connectors that were not requested are disabled with a leading # (#connector-kafka) instead of being deleted. install-plugin.sh already skips # lines, so the installed result is the same, but the full connector list and its section headers stay in the file, and a later call can enable a different connector without reconstructing the file. @SEZ9 confirmed this approach on #12707.

Scope: two new files under tools/seatunnel-python-sdk. The helper is not exported from seatunnel/__init__.py, and install-plugin.sh, plugin_config and the engine are untouched. Import/usage docs will be added in the launcher PR that exposes the helper to users.

Does this PR introduce any user-facing change?

No. It is an internal helper that nothing calls yet, and it is not part of the SDK's public API.

How was this patch tested?

Added 39 unit tests in tools/seatunnel-python-sdk/tests/test_plugin_installer.py. They use a temporary directory and mock subprocess.run / shutil.which, so no network or real distribution is needed. They cover:

Only the requested connectors stay enabled, and the others are disabled with #
License header, comments and section header preserved
Idempotence: a second run is byte-identical and the file is not rewritten (test_second_run_with_same_connectors_is_idempotent)
Re-enable: connector A, then connector B, ends with A commented and B active (test_previously_disabled_connector_can_be_enabled_again)
Short name vs artifact ID, duplicate names, a single string argument
Whitespace around connector IDs, and lines the script would reject raising with their line number before any rewrite
Unknown connector, empty request, invalid names, None / non-string names and a non-UTF-8 file raise PluginInstallError, and the file is left unchanged
CRLF line endings preserved, and a last line without a trailing newline
Mocked install-plugin.sh call: the config is rewritten before the script runs with [bash, script, version] (test_runs_install_script_with_version_after_selecting), and a successful install keeps the new selection
Rollback: a non-zero exit, an OSError, a KeyboardInterrupt, and an interrupt right after the rewrite all restore plugin_config byte for byte, including an earlier non-default selection with CRLF line endings; an unchanged file is not rewritten on failure
Failed restore: a regular error reports both failures; KeyboardInterrupt / SystemExit are re-raised with the restore failure attached
Pre-flight checks: Windows, an empty or invalid version, an invalid seatunnel_home, a relative home, a missing bash, script or plugin_config, and an unknown connector all fail without running the script or rewriting the file
Lock: another process cannot take the lock while the script runs (test_lock_is_held_while_the_script_runs, POSIX only)

Results: all 67 SDK tests (39 new + 28 existing) pass on Linux (Python 3.9, 3.10, 3.13, including the lock test) and on Windows (Python 3.9, 3.11, 3.13; the lock test is skipped without fcntl).

I also ran install_plugins on Linux against copies of the real config/plugin_config and bin/install-plugin.sh:

Successful install: only # toggles changed compared with the original file, the section header was kept, a second run was byte-identical, and connector-assert-3.0.0.jar was downloaded and passed the script's checksum check.
Relative seatunnel_home: install_plugins('dist', ['assert'], '3.0.0') installed connector-assert-3.0.0.jar.
Failed install (version 3.0.1, which returns 404): PluginInstallError was raised, and plugin_config was byte-identical to the original afterwards, with no temporary files left behind.
Two concurrent install_plugins calls on one copy were serialized, and each script saw only its own selection.

Side note, not changed here: the default version in install-plugin.sh is 3.0.1, which is not on Maven Central yet (404). Only 3.0.0 is published.

CI note: the Test with Python 3.9 check currently fails at "Set up Python" because ubuntu-latest is now Ubuntu 26.04, which has no Python 3.9 build. That is fixed separately in #12994.

To run the tests (from tools/seatunnel-python-sdk/):

python -m pytest tests/ -v

Check list

If any new Jar binary package adding in your PR, please add License Notice according
New License Guide
If necessary, please update the documentation to describe the new feature. https://github.com/apache/seatunnel/tree/dev/docs
If necessary, please update incompatible-changes.md to describe the incompatibility caused by this PR.
If you are contributing the connector code, please check that the following files are updated:
1. Update plugin-mapping.properties and add new connector information in it
2. Update the pom file of seatunnel-dist
3. Add ci label in label-scope-conf
4. Add e2e testcase in seatunnel-e2e
5. Update connector plugin_config

DanielLeens

Thanks for the careful test coverage. I reviewed the full two-file diff together with bin/install-plugin.sh and the real config/plugin_config parser.

What Problem Does This PR Solve?

User pain point: Python callers need a safe way to select connector artifacts and invoke the distribution installer.
Fix approach: normalize connector names, rewrite config/plugin_config, then call bin/install-plugin.sh.
One-sentence summary: the helper exposes a package-level path for selecting plugins and starting the existing installer.

1. Code Change Review

1.1 Core Logic Analysis

Before this change, a Python caller had to mutate the distribution configuration and invoke the shell script itself. After this change, install_plugins() validates the layout, calls select_plugins(), then executes the script.

install_plugins(home, connectors, version)
  -> select_plugins(config/plugin_config, connectors)
      -> atomically rewrites enabled connector lines
  -> bin/install-plugin.sh <version>
      -> reads the now-selected lines
      -> downloads each selected artifact into connectors/

Normal execution reaches both new functions. The name validation, line-ending preservation, duplicate-request handling, and subprocess argument list are sound. However, the configuration rewrite is committed before the shell installer starts.

Issue 1: A failed install leaves `plugin_config` changed

Location: tools/seatunnel-python-sdk/seatunnel/plugin_installer.py:164-174
Problem description: install_plugins() calls select_plugins() before subprocess.run(). The rewrite is atomic only at the file-write level; if install-plugin.sh later fails because a download, checksum, Maven resolution, or version validation fails, the function raises but leaves the distribution configured with a different enabled-plugin set. This is reachable on the normal install path because the shell script can fail after it reads the selected config.
Potential risk: A failed install has a persistent, surprising side effect. A later retry or manual invocation can install a different set of connectors than the distribution had before the failed call, and partial artifact downloads may coexist with that changed selection.
Root cause and current change location: The change point is the Python orchestration layer, which owns both the pre-install rewrite and the error boundary. The root cause is treating the configuration write as independent from the shell operation even though it is part of one user-visible install transaction.
Best improvement: Preserve the original file bytes and mode before selection. If the script exits non-zero or cannot start, restore the original file atomically before raising; add a regression test that makes subprocess.run() return non-zero and asserts byte-identical restoration. Option A is rollback in install_plugins() (smallest safe fix and recommended). Option B is refactoring the shell script to accept an explicit selected-artifact list without changing plugin_config; that is architecturally cleaner but changes the existing shell contract and has a much larger compatibility and verification cost.
Severity: Medium
Raised by another reviewer: No

1.2 Compatibility Impact

Partially incompatible operationally: no existing Python API is changed, but the new helper mutates the distribution's shared plugin_config. The rollback above is needed to make failure behavior compatible with caller expectations of an unsuccessful operation.

1.3 Performance / Side-Effect Analysis

Connector selection is linear in the config file and process execution delegates the expensive work to the existing installer. There is no new concurrency, retry, network, lock, or memory hot path. The relevant side effect is Issue 1's persistent configuration mutation on failure.

1.4 Error Handling and Logging

Issue 1 is the formal error-path issue. The returned exit-code exception has useful connector context, and using a list-form subprocess.run() avoids shell injection. Script output is intentionally streamed rather than swallowed.

2. Code Quality Assessment

2.1 Coding Standards

Naming, license headers, docstrings, and the small helper boundaries are clear. The atomic-file-write helper correctly retains file mode and removes its temporary file on write failure.

2.2 Test Coverage and Test Stability

The new unit tests cover normalization, malformed input, LF/CRLF preservation, missing prerequisites, and subprocess failures. Stability rating: Stable. They use isolated temporary directories and mocks rather than timing, network, or fixed-port dependencies. Coverage is incomplete for Issue 1: test_nonzero_exit_raises checks only the exception, not that the pre-install configuration is restored.

Local build/test: not executed. This is a PR review; the evidence is the fetched diff, the unit tests, the real installer call chain, and the current CI snapshot. The Build check was still in progress when this review was prepared.

2.3 Documentation Updates

Because the module is packaged and importable as seatunnel.plugin_installer, add a short SDK usage/API note when the helper becomes part of the launcher flow. No end-user connector documentation is required for this internal integration step.

3. Architectural Soundness

3.1 Elegance of the Solution

The selected change location is appropriate: the Python SDK should orchestrate the existing distribution script rather than duplicate its download logic. The rollback gap prevents this from being a complete long-term solution.

3.2 Maintainability

Keeping parsing and atomic writes in separate helpers is maintainable. Restoring a saved original on script failure keeps the ownership boundary equally clear.

3.3 Extensibility

Option A preserves the current script contract for future launcher work. Option B can be considered later if the shell installer gains an artifact-list interface.

3.4 Historical-Version Compatibility

No persisted engine state, protocol, or configuration schema changes. The only compatibility concern is the local distribution file side effect described in Issue 1.

4. Issue Summary

Number	Issue	Location	Severity
1	Failed install leaves `plugin_config` changed	`plugin_installer.py:164-174`	Medium

5. Merge Recommendation

Conclusion: Ready to merge after fixes

Blockers — must be fixed
- Issue 1: restore the pre-call plugin_config whenever the installer cannot start or returns a non-zero exit status, and add the corresponding regression test.
Recommended fixes — non-blocking
- Document the import/API once the launcher exposes this helper to SDK users.

Overall assessment: reusing the hardened shell installer is the right minimal design, and the tests are otherwise well targeted. The rollback fix is smaller and safer than expanding the shell-script interface, so I recommend Option A for this PR.

karthik-0306 · 2026-10-18T12:18:45Z

Thanks @DanielLeens for the review. Issue 1 is fixed with Option A in the latest commit:

install_plugins() saves the exact bytes of plugin_config before selecting, and restores them atomically if install-plugin.sh exits non-zero, fails to start, or is interrupted (Ctrl+C during a download). The original error is still raised; if the restore itself fails, the message includes both errors. A successful install keeps the new selection.
Regression tests: the non-zero exit and OSError tests now assert byte-identical restoration, plus new tests for restoring a previous (non-default, CRLF) selection, KeyboardInterrupt, a failed restore, and a successful install keeping the selection. The new restore tests fail on the previous commit and pass now.

All 54 SDK tests pass on Python 3.9, 3.11 and 3.13. I also ran it against the real install-plugin.sh with a version that 404s on Maven Central: the call raised and plugin_config was byte-identical to the original afterwards.

On the non-blocking note: agreed, I'll document the import/API in the launcher PR that exposes it.

DanielLeens

Thanks for the quick follow-up, @karthik-0306, and for addressing the rollback point so thoroughly. I re-read the new commit (0aa9f223a) against the installer script and the previous revision.

What Problem Does This PR Solve?

User pain point: Python callers need a safe way to select connector artifacts in config/plugin_config and invoke bin/install-plugin.sh.
Fix approach: normalize names, rewrite plugin_config atomically, then run the existing installer; on this head the original file bytes are restored if the installer fails.
One-sentence summary: a small, well-tested Python helper that wraps the existing installer without duplicating its download logic.

1. Code Change Review

1.1 Core Logic Analysis

Before the update, install_plugins() committed the plugin_config rewrite before subprocess.run() and left it in place when the script failed (my previous Issue 1). Now:

install_plugins(home, connectors, version)
  -> read original bytes of config/plugin_config           (plugin_installer.py:168-174)
  -> select_plugins(...)                                    (atomic rewrite)
  -> subprocess.run([bash, install-plugin.sh, version])
       any exception / non-zero exit / KeyboardInterrupt
         -> _write_atomically(plugin_config, original)      (plugin_installer.py:187-194)
         -> re-raise the original error

The fix is at the right layer (the Python orchestration owns both the rewrite and the error boundary). I checked that the byte round trip is lossless: select_plugins reads with newline="" and now writes UTF-8 bytes, so CRLF and LF files are preserved exactly, and the restore writes the saved raw bytes through the same temp-file plus os.replace path with the file mode copied. A failure inside select_plugins itself never reaches the restore block, which is correct because that function does not modify the file on error.

1.2 Compatibility Impact

Fully compatible. This is a new module; no existing Python API, config option or shell contract changes.

1.3 Performance / Side-Effect Analysis

One extra file read and, only on failure, one extra atomic write. No new threads, locks, retries or network calls.

1.4 Error Handling and Logging

The previous blocker is resolved: non-zero exit, OSError on start and KeyboardInterrupt all restore the file, and a failed restore reports both errors.

Issue 1 (non-blocking): a failed restore during Ctrl+C turns the interrupt into `PluginInstallError`

Location: tools/seatunnel-python-sdk/seatunnel/plugin_installer.py:187-194
Problem: if the restore itself fails while handling a KeyboardInterrupt, the code raises PluginInstallError, so the interrupt is swallowed and the caller's loop may continue.
Potential risk: very low and only on a double failure; the message still contains both errors.
Best improvement: optionally re-raise KeyboardInterrupt/SystemExit after attaching the restore failure as context (for example via logging or add_note on newer Pythons).
Severity: Low
Raised by another reviewer: No

2. Code Quality Assessment

2.1 Coding Standards

Naming, docstrings, license headers and helper boundaries are clear; the new comment explains why BaseException is caught.

2.2 Test Coverage and Test Stability

The tests were strengthened rather than weakened: the non-zero exit and OSError cases now assert byte-identical restoration, and new tests cover restoring a non-default CRLF selection, KeyboardInterrupt, a failing restore, and a successful install keeping the selection. They use temporary directories and mocks only. Stability rating: Stable.

2.3 Documentation Updates

No user-facing connector docs are affected. As agreed, documenting the import/API in the launcher PR is fine.

3. Architectural Soundness

Restoring the saved bytes keeps the existing shell-script contract intact and is the smaller, safer option I recommended. Maintainability and extensibility are fine for the follow-up launcher work.

4. Issue Summary

Number	Issue	Location	Severity
1	Failed restore during Ctrl+C converts the interrupt into `PluginInstallError`	`plugin_installer.py:187-194`	Low

5. Merge Recommendation

CI for the current head 0aa9f223a: Build, labeler and Notify test workflow are all green. The branch is 13 commits behind dev but has no conflicts and CI is green, so no sync is needed.

Conclusion: Ready to merge

Blockers — must be fixed
- None. My earlier blocker (restore plugin_config on failure) is fixed and covered by regression tests, and CI is green.
Recommended — non-blocking
- Issue 1 above, at your discretion.

Thanks again for the careful work.

karthik-0306 · 2026-10-19T09:11:58Z

Thanks @DanielLeens for the approval. I've also addressed the non-blocking Issue 1 in the latest commit: if the restore fails while handling KeyboardInterrupt / SystemExit, the original interrupt is now re-raised (so the caller still stops), with the restore failure attached as its __context__ (and as a note on Python 3.11+). Regular errors still raise a single PluginInstallError with both messages. Covered by test_restore_failure_during_interrupt_keeps_the_interrupt. All 55 SDK tests pass on Python 3.9, 3.11 and 3.13.

DanielLeens

Thanks a lot for the quick follow-up, @karthik-0306, and for picking up my earlier non-blocking Issue 1 so promptly. I re-read the full current diff at head 5e7780043 (the incremental change since my approval at 0aa9f223a touches only the module docstring, two param docs, and the restore-failure branch) together with the existing tests.

What Problem Does This PR Solve?

User pain point: Python callers need a safe way to select connector artifacts in config/plugin_config and invoke bin/install-plugin.sh.
Fix approach: normalize names, rewrite plugin_config atomically, run the existing installer, and restore the original bytes if the installer fails or is interrupted.
One-sentence summary: a small, well-tested Python wrapper around the existing installer that does not duplicate its download logic.

1. Code Change Review

1.1 Core Logic Analysis

Delta since my last review:

install_plugins(home, connectors, version)
  -> save original bytes -> select_plugins() -> subprocess.run(install-plugin.sh)
       on any BaseException:
         -> _write_atomically(plugin_config, original)
              restore OK      -> re-raise the original error
              restore FAILS   -> Exception:           PluginInstallError(both messages) from exc
                                 KeyboardInterrupt/SystemExit: add_note (3.11+) then `raise exc`
                                 (restore error becomes exc.__context__)

Key findings:

The Low issue from my previous review is resolved: an interrupt is no longer converted into a regular PluginInstallError, so a caller loop still stops on Ctrl+C. The isinstance(exc, Exception) split in plugin_installer.py is the right discriminator.
The hasattr(exc, "add_note") guard keeps Python 3.9/3.10 working, and raise exc inside the except PluginInstallError block preserves the restore error as __context__.
The new module docstring and the :param text now state clearly that these helpers rewrite plugin_config in place and must only be pointed at a caller-owned copy (for example the launcher's per-user, per-version cache). This is a good ownership boundary to write down before the launcher PR lands.

1.2 Compatibility Impact

Fully compatible. This is a new module; no existing Python API, config option or shell contract changes.

1.3 Performance / Side-Effect Analysis

No change from the previous head: one extra file read, and one extra atomic write only on failure. No new threads, locks, retries or network calls.

1.4 Error Handling and Logging

No new issues found. The double-failure path now preserves both pieces of information for both exception families.

2. Code Quality Assessment

2.1 Coding Standards

License header, docstrings and helper boundaries are clear; the comment on the interrupt branch explains why the original exception is re-raised.

2.2 Test Coverage and Test Stability

test_restore_failure_during_interrupt_keeps_the_interrupt covers both KeyboardInterrupt and SystemExit via subTest, asserts the re-raised type, the __context__ restore error, and the __notes__ content on Python 3.11+. The previous test was only refactored into a shared _patch_failing_restore helper with identical assertions, so strictness is not weakened. Tests use temporary directories and mocks only. Stability rating: Stable.

2.3 Documentation Updates

No user-facing connector docs are affected; documenting the import/API in the launcher PR was agreed earlier.

3. Architectural Soundness

3.1 Elegance: Precise fix. 3.2 Maintainability: the ownership note in the module docstring prevents a likely misuse. 3.3 Extensibility: fine for the follow-up launcher work. 3.4 Historical-Version Compatibility: not applicable (new module).

4. Issue Summary

Number	Issue	Location	Severity
-	No open issues	-	-

5. Merge Recommendation

CI for the current head 5e7780043: labeler and Notify test workflow are green; Build was still queued when I checked. The PR has no merge conflicts (it is 13 commits behind dev, which is not needed for this Python-only change). Please let Build finish green before merging.

Conclusion: Ready to merge

Blockers — must be fixed
- None on the code side.
Recommended — non-blocking
- None.

Thanks again for the careful work.

karthik-0306 · 2026-10-20T18:01:48Z

@SEZ9 @DanielLeens a quick CI update: Build on the current head (5e77800) keeps failing in Run / all-connectors-it-6 (17, ubuntu-latest) (attempts 1 and 3; attempt 1 also failed unit-test (11)).

I don't think this comes from this PR:

that shard only runs the Java e2e modules assert, databend, fake, fluss, hudi, nats-jetstream, neo4j, paypal, sentry; this PR only adds two Python files under tools/seatunnel-python-sdk;
Build was green on the previous head 0aa9f22 (Oct 18), and the change since then is docstrings plus the restore-failure branch in Python;
the scheduled Backend run on dev itself (#5144, Oct 19) also fails all-connectors-it-6 and many other IT and unit-test jobs.

Should I keep re-running, rebase on the latest dev, or wait for the dev CI to be fixed? Happy to do whichever you prefer.

DanielLeens · 2026-10-21T00:40:53Z

Thanks for the clear CI update, @karthik-0306, and for asking before re-running again. I looked at the run logs, and my conclusion is that this failure is not caused by your two Python files, but re-running the same head will not make it pass, so please do not keep retrying.

What actually fails. In the fork run for 5e77800 (run 39227503024), attempt 3 of all-connectors-it-6 (17, ubuntu-latest) fails with DatabendCDCSinkIT.testDatabendSinkCDC:88 (ConditionTimeoutException ... Expected 3 records in sink_table ==> expected: <3> but was: <0> within 3 minutes). Attempt 1 of the same job failed on exactly the same test and assertion, so it is deterministic on this head rather than a flake. It is a Databend connector E2E, nothing in tools/seatunnel-python-sdk is on its path.

Why dev passes. This symptom is the one fixed by #12848 (Run the final Databend CDC merge when the writer closes, merged on Oct 19 as 2bc9d0e; its description names DatabendCDCSinkIT failing with expected: <3> but was: <0>). Your head is based on an older dev and does not contain that commit (the branch is 21 commits behind dev). One correction to the dev comparison: the scheduled Backend run on dev you linked (run 39282837181, head 03f3f3e, which already includes #12848) passed all-connectors-it-6 on both JDK 11 and 17. Its red jobs were unit-test, engine-resource-managers-it and connector-redis-it, so all-connectors-it-6 is not currently broken on dev.

Attempt 1 unit-test (11). That one failed in SavePointTest.testSavePointButJobGoingToFail (engine module), which is also unrelated to a Python-only change.

Concrete next step: merge (or rebase) the latest dev into your branch and push. That picks up #12848, and the push starts a fresh Build in your fork, so you do not need to re-run the old attempts. Please note that unit-test on current dev can still show a JdbcOptionRuleCompletenessTest failure until #12875 lands; that is also unrelated to this PR, so if all-connectors-it-6 turns green and only that shows up, just say so here.

My review of the code is unchanged: no blocking issue on the Python side, and I am happy to approve once Build is green on the synced head. Thank you for the careful follow-up!

DanielLeens

Thanks for the rebase, @karthik-0306! I went through the full diff again at head 8e615146d from scratch, and I also owe you a small apology: in my last two rounds I missed one low-severity item (Issue 1 below). It is a carryover from the earlier versions, not something introduced by this push, and it does not block the merge. Sorry for missing it earlier, that one is on me.

What Problem Does This PR Solve?

User pain point: callers of the Python SDK (and the upcoming pip-installable launcher from #12707) have no safe way to pick connector artifacts in config/plugin_config and run bin/install-plugin.sh; doing it by hand means editing a shared file and leaving it modified if the download fails.
Fix approach: select_plugins() normalizes names and rewrites plugin_config atomically (disable with a leading #, never delete lines); install_plugins() validates the layout and bash first, runs the existing script, and restores the original bytes if the script fails, cannot start, or is interrupted.
One-sentence summary: a small, well-tested Python wrapper around the existing installer that reuses its download logic instead of duplicating it.

1. Code Change Review

1.1 Core Logic Analysis

What changed since my last review (5e778004): the branch was rebased onto the current dev (14a27ac26; the PR now has three commits, behind_by = 0). I diffed tools/seatunnel-python-sdk between 5e778004 and this head and it is byte-identical, so the two Python files are exactly what I reviewed last time. The PR touches only tools/seatunnel-python-sdk/seatunnel/plugin_installer.py and tools/seatunnel-python-sdk/tests/test_plugin_installer.py; nothing under dev's Java modules is changed.

Runtime path:

install_plugins(home, connectors, version)
  -> validate version, layout (plugin_config, install-plugin.sh), bash         (plugin_installer.py:160-176)
  -> read original bytes                                                        (:178-184)
  -> select_plugins(): normalize -> rewrite lines -> atomic write only if changed (:70-120)
  -> subprocess.run([bash, script, version], cwd=home)                          (:188-198)
       failure / OSError / BaseException -> _write_atomically(original)         (:199-212)

I re-checked this against the real consumer, bin/install-plugin.sh:264-282: it skips lines whose first character is - or # and rejects any other line containing characters outside [A-Za-z0-9._-]. The Python regexes (plugin_installer.py:37, :42) use the same character set, so a line the Python code enables is exactly a line the script will download, and #connector-x is exactly what the script skips. The name validation also blocks a leading -, so a request cannot clobber a section header. The restore branch (:199-212) handles all three exit paths and keeps KeyboardInterrupt / SystemExit as interrupts, which is the right behavior for a caller loop.

Resolved from my earlier rounds: the Low item about interrupts being converted into PluginInstallError (fixed in 5e778004, still intact); the ownership/in-place-rewrite documentation (module docstring :18-21 and the :param text). No inline review threads or other reviewers' reviews exist on this PR, so there is nothing to dedupe against; the only other activity is your CI question and my earlier answer on the issue thread.

1.2 Compatibility Impact

Fully compatible. This adds a new module only. No existing Python API, config option, default, protocol, serialization format, or shell contract changes, and bin/install-plugin.sh and config/plugin_config are not modified. No entry is needed in incompatible-changes.md.

1.3 Performance / Side-Effect Analysis

Negligible. One extra read of a small text file, and one extra atomic write only when the selection changes or on rollback. No threads, locks, retries or network calls of its own; the script's output is intentionally not captured, so there is no unbounded buffer. Atomicity: the temp file is created in the same directory and swapped with os.replace, with the mode copied, and removed on any failure. Idempotent: a second identical call does not rewrite the file.

1.4 Error Handling and Logging

Exceptions carry context (path, exit code, connector list) and are chained with from exc; nothing is swallowed and no sensitive data is logged. The double-failure case reports both errors.

Issue 1: Rewritten CRLF plugin_config would be rejected by install-plugin.sh

Location: tools/seatunnel-python-sdk/seatunnel/plugin_installer.py:98-109 (consumer: bin/install-plugin.sh:264-272)
Problem description: select_plugins() deliberately preserves CRLF line endings (and test_crlf_line_endings_are_preserved asserts it). The script, however, reads each line with read -r and rejects any line containing a character outside [A-Za-z0-9._-], which includes a trailing \r. So if plugin_config ever has CRLF endings (for example a Windows checkout with core.autocrlf=true), the enabled connector-x\r line would make the script fail with invalid connector artifact ID, and the failure is then rolled back. This is true for the unmodified file as well, so it is a pre-existing limitation of the script, not a regression from this PR.
Potential risk: low. The launcher is expected to use its own per-version cache copy of the distribution (LF endings), so this is unlikely to be hit in practice. When hit, the user sees a confusing script error with no hint about line endings.
Best improvement: Option A (preferred, no code change here): keep the current byte-preserving behavior and add one sentence to the docstring saying CRLF files are preserved but install-plugin.sh itself requires LF endings. Option B: normalize to LF when rewriting; I would not do this because it breaks the byte-identical/idempotency guarantee you test for.
Severity: Low
Raised by another reviewer: No

2. Code Quality Assessment

2.1 Coding Standards

ASF license headers are present on both files, public functions and the non-obvious helpers have docstrings, and the comments explain the why (validate before rewriting, interrupt handling). No missing documentation on core methods or non-trivial fields.

2.2 Test Coverage and Test Stability

test_plugin_installer.py (352 lines) covers selection and idempotency (:77-134), CRLF and no-trailing-newline (:159-177), invalid and unknown names (:136-157), the script contract and call arguments (:198-213), exit-code / OSError / KeyboardInterrupt rollback (:221-258), double failure for both exception families (:275-302), and the pre-flight checks that must not touch the file (:304-348). Deleting the restore branch or the pre-flight ordering would turn several of these red, so they have real failure power. Tests use TemporaryDirectory, mocks for which and subprocess.run, no sleeps, no ports and no shared global state (the module-level PLUGIN_CONFIG is an immutable string).
Stability rating: Stable (no timing, ordering, network or environment dependence; evidence: setUp/tearDown at :45-57, all subprocess interaction mocked at :188-196).

2.3 Documentation Updates

No docs/en / docs/zh change is needed for this PR because it adds an internal SDK helper rather than a user-facing connector option, and the SDK README does not yet list the module. Documenting the public import in the launcher PR was already agreed, so I am not asking for it here.

3. Architectural Soundness

3.1 Elegance of the Solution

Precise fix. It wraps the existing script instead of reimplementing download/checksum logic.

3.2 Maintainability

Clear boundaries; the ownership note in the module docstring prevents the most likely misuse (rewriting a shared installation or a source checkout).

3.3 Extensibility

Good base for the launcher work. The regex is intentionally coupled to the script's accepted character set, which is documented in the comments.

3.4 Historical-Version Compatibility

Not applicable: a new module with no persisted state or protocol. Python 3.9 compatibility is kept (add_note is guarded by hasattr).

4. Issue Summary

Number	Issue	Location	Severity
1	CRLF-preserved `plugin_config` is rejected by `install-plugin.sh` (pre-existing script limitation; docs note suggested)	`plugin_installer.py:98-109`	Low

5. Merge Recommendation

CI state, checked on the current head 8e615146d: the apache-side PR signal and SeaTunnel Python SDK workflows are action_required (they need a maintainer to approve the run, they did not execute), and the real Build run in your fork (https://github.com/karthik-0306/apache-seatunnel/actions/runs/40004598895) was still in_progress when I looked: 80 jobs green, Run / unit-test (17, windows-latest) red, and kafka-connector-it (x2) and connector-redis-it still running. The red Windows job fails in RestApiHttpBasicTest (seatunnel-engine-server, a 300s timeout), which is an engine test that nothing in this Python-only change can reach, and it passed on the same head for unit-test (11, windows-latest). So this looks like a runner flake rather than a PR problem (this is my inference from the log, not something I reproduced). Please do not change any code for it; just re-run only the failed job once Build finishes.

Conclusion: Ready to merge

Blockers — must be fixed
- None on the code side. The only remaining gate is CI: Build must finish green on this head (after a job-level re-run of the Windows unit-test if it stays red), and a maintainer needs to approve the apache-side SeaTunnel Python SDK workflow so the SDK tests actually run.
Recommended fixes — non-blocking
- Issue 1 (Low): one docstring sentence about LF requirements of install-plugin.sh. Fine to take or leave.

Overall assessment: the approach is sound, the rollback semantics are correct on all three exit paths, and the tests are deterministic and meaningful. No better alternative implementation is needed. Happy to approve as soon as Build is green on this head. Thanks again for the careful, responsive work!

karthik-0306 · 2026-10-24T08:05:45Z

Thanks @DanielLeens, and please don't apologize! Catching something subtle like this on a later pass is exactly why a fresh re-review is useful, and it's a real catch that I'd missed too.

I took Option A in the latest commit: the select_plugins() docstring now notes that CRLF line endings are preserved, but bin/install-plugin.sh only accepts LF endings (a connector line ending in \r is rejected as an invalid artifact ID), so callers should pass a plugin_config with LF endings as shipped in the distribution. Docstring only, no behavior change; all 55 SDK tests still pass on Python 3.9, 3.11 and 3.13.

On CI: I'll re-run only the failed Windows unit-test job if it is still red on the new head, without touching the code.

DanielLeens

Thanks for the quick follow-up, @karthik-0306, and for the kind words. I re-reviewed the full diff at head ebe122b23. The only change since my last review (8e615146d) is the new paragraph in the select_plugins() docstring (tools/seatunnel-python-sdk/seatunnel/plugin_installer.py:78-81) stating that CRLF endings are preserved but bin/install-plugin.sh only accepts LF endings. That fully resolves the Low Issue 1 from my previous round, with no behavior change; the rest of both files is byte-identical to the version I reviewed.

What Problem Does This PR Solve?

User pain point: callers of the Python SDK (and the upcoming pip-installable launcher from #12707) have no safe way to select connector artifacts in config/plugin_config and run bin/install-plugin.sh; doing it by hand leaves the file modified if the download fails.
Fix approach: select_plugins() normalizes names and rewrites plugin_config atomically (disabling with a leading #, never deleting lines); install_plugins() validates the layout and bash first, runs the existing script, and restores the original bytes if the script fails, cannot start, or is interrupted.
One-sentence summary: a small, well-tested Python wrapper around the existing installer that reuses its download logic instead of duplicating it.

1. Code Change Review

1.1 Core Logic Analysis

install_plugins(home, connectors, version)                        plugin_installer.py:146
  -> validate version, plugin_config, install-plugin.sh, bash      (before any write)
  -> save original bytes of plugin_config
  -> select_plugins(): normalize -> toggle '#' per line -> atomic write only if changed   :70
  -> subprocess.run([bash, script, version], cwd=home)             :195
       non-zero exit / OSError / BaseException -> _write_atomically(original)   :204
            restore fails: Exception -> PluginInstallError(both errors); KeyboardInterrupt/SystemExit re-raised

The character sets in _ARTIFACT_ID_RE / _CONNECTOR_LINE_RE (:37, :42) match what bin/install-plugin.sh accepts and skips, so an enabled line is exactly a line the script downloads. The new docstring correctly explains the remaining CRLF limitation, which lives in the script, not in this helper.

1.2 Compatibility Impact

Fully compatible. New module only; no existing Python API, config option, default, shell contract or engine code changes.

1.3 Performance / Side-Effect Analysis

Negligible: one extra read of a small file, and one extra atomic write only when the selection changes or on rollback. No threads, locks, retries or network calls of its own; script output is streamed, not buffered.

1.4 Error Handling and Logging

Exceptions carry path, exit code and connector context and are chained with from exc; nothing is swallowed and no sensitive data is logged. No formal issues.

2. Code Quality Assessment

2.1 Coding Standards

ASF headers on both files; public functions and non-obvious helpers have docstrings that explain the why (ownership boundary, validation before rewrite, interrupt handling, and now the LF requirement).

2.2 Test Coverage and Test Stability

Unchanged since my last review: selection and idempotency, CRLF and no-trailing-newline handling, invalid/unknown names, the script call contract, rollback for exit code / OSError / KeyboardInterrupt, double failure for both exception families, and pre-flight checks that must not touch the file. Removing the restore branch or the pre-flight ordering turns several tests red. Stability rating: Stable (temporary directories and mocks only; no sleeps, ports, network or shared mutable state).

2.3 Documentation Updates

No docs/en / docs/zh change is needed for this internal SDK helper; documenting the public import in the launcher PR was agreed earlier.

3. Architectural Soundness

3.1 Elegance

Precise fix: it wraps the existing script instead of reimplementing download and checksum logic.

3.2 Maintainability

Clear boundaries; the ownership and LF notes in the docstrings prevent the two most likely misuses.

3.3 Extensibility

Good base for the launcher work in #12707.

3.4 Historical-Version Compatibility

Not applicable: new module with no persisted state or protocol; Python 3.9 support is kept.

4. Issue Summary

Number	Issue	Location	Severity
-	No open issues (previous Low Issue 1 resolved in `ebe122b23`)	-	-

5. Merge Recommendation

CI for head ebe122b23: the apache-side SeaTunnel Python SDK and PR signal workflows are action_required, so a maintainer still needs to approve them for the SDK tests to run upstream. The real Build run in your fork (run 40076614568) is still in progress; so far 56 jobs are green and one is red: engine-v2-it (11, ubuntu-latest), failing in BackpressureSlowSinkIT.testCheckpointsKeepCompletingUnderSustainedBackpressure. That is a Zeta engine E2E that nothing in this Python-only change can reach. It is a known intermittent failure on dev whose fix (#12316) is still open, so syncing with the latest dev would not help here. Once the run finishes, a job-level re-run of only that job (as you planned) is the right next step.

Conclusion: Ready to merge

Blockers - must be fixed
- None on the code side. Remaining gates are CI only: Build green on this head (after re-running the unrelated engine-v2-it (11) job) and a maintainer approving the apache-side SeaTunnel Python SDK workflow.
Recommended fixes - non-blocking
- None.

Overall assessment: No blocking issues found. Residual risk is limited to CI, and the failing job is unrelated to this diff. I am happy to approve once Build is green on this head. Thank you for the careful and responsive work!

DanielLeens · 2026-10-25T12:46:31Z

Thanks for the contribution. The CI/merge fact in my previous review has changed. The exact-head fork Build is now completed with failure for ebe122b2359030f467dcd6cd8156fa2dbd2aabab. This is a gate correction, not a second full source review of that unchanged head.

The Java 11 engine job failed in BackpressureSlowSinkIT.testCheckpointsKeepCompletingUnderSustainedBackpressure, observing only two additional completed checkpoints within 90 seconds. This replaces my earlier in-progress run description; it is the same engine failure already discussed, and the Python helper cannot exercise that engine locking path.

The test and relevant upstream checkpoint-handoff fix status were rechecked: #12316 remains open, and no corresponding fix is in latest dev. Syncing now is not a demonstrated cure. Please rerun that failed job, and retain the new log with the shared engine investigation if it repeats; avoid unrelated SDK changes. Apache-side workflow approval for the Python SDK tests remains a separate maintainer action if still pending.

Comparison against dev is diverged, ahead 4 / behind 13; merge conflicts: no. Drift is recorded independently and does not itself justify a sync request. No new source-level blocker from my previous full review is asserted here; a successful required Build and write-capable maintainer approval remain separate gates. Daniel has READ permission, so this comment is not an effective approval. Thanks for keeping the fix focused.

karthik-0306 · 2026-10-25T16:53:30Z

Thanks @DanielLeens! I re-ran only the failed engine-v2-it (11, ubuntu-latest) job and it passed, so the fork Build is now green on the current head ebe122b (run 40076614568). No code changes.

@SEZ9 this is ready for your review whenever you have time. Could a maintainer also approve the pending apache-side SeaTunnel Python SDK workflow run, so the SDK tests run here too? Thanks!

DanielLeens

Thanks for re-running only the failed job, @karthik-0306, and for keeping the code untouched. CI was previously the only remaining blocker; CI is now passing, so the conclusion is being changed to approve.

I re-confirmed the live state at head ebe122b2359030f467dcd6cd8156fa2dbd2aabab (unchanged since my last full review): the fork Build run 40076614568 is completed / success on attempt 2 for exactly this SHA. The only red job on attempt 1 was engine-v2-it (11, ubuntu-latest) (BackpressureSlowSinkIT), a Zeta engine E2E that this Python-only change cannot reach, and it passed on the job-level re-run.

What Problem Does This PR Solve?

User pain point: Python callers need a safe way to select connector artifacts and run bin/install-plugin.sh from the SDK without corrupting the shipped config/plugin_config.
Fix approach: new plugin_installer.py in tools/seatunnel-python-sdk that selects plugins, invokes the installer, and restores the exact original plugin_config bytes if the install fails or is interrupted.
One-sentence summary: a small, self-contained SDK helper with strong rollback semantics and matching tests.

1. Code Change Review

1.1 Core Logic Analysis

Selection, byte-exact snapshot, atomic restore on non-zero exit / start failure / KeyboardInterrupt / SystemExit, and the LF-endings note in the select_plugins() docstring (tools/seatunnel-python-sdk/seatunnel/plugin_installer.py:78-81) are all in place. No source change since my last full review.

1.2 Compatibility Impact

Fully compatible: new Python module only; no Java, engine, config option or protocol change; Python 3.9 support is kept.

1.3 Performance / Side-Effect

Negligible. The only side effect is the plugin_config rewrite, which is restored on failure.

1.4 Error Handling and Logging

Original error is preserved; restore failures are attached (or raised together for regular errors). Interrupts are re-raised. No open issues.

2. Code Quality Assessment

2.1 Coding Standards

Clear docstrings on the public API and the ownership/LF boundaries.

2.2 Tests and Stability

55 SDK tests cover success, non-zero exit, start failure, byte-identical restore (including CRLF), interrupt, and failed restore; they are deterministic and use no network. Stability: High.

2.3 Documentation

Docstring-level docs are sufficient for this internal SDK helper; the launcher PR (#12707) is the right place for user-facing docs, as agreed.

3. Architectural Soundness

3.1 Clear single responsibility. 3.2 Maintainable, no engine/core coupling. 3.3 Good base for the launcher work. 3.4 Historical-version compatibility: not applicable, no persisted state or protocol.

4. Issue Summary

Number	Issue	Location	Severity
-	No open issues	-	-

5. Merge Recommendation

Conclusion: Ready to merge

Blockers - must be fixed
- None. The apache-side SeaTunnel Python SDK workflow still shows as pending maintainer approval, so a write-access maintainer needs to approve that run (and the final merge).
Recommended fixes - non-blocking
- None.

Overall assessment: no source-side or CI blocker remains on this head. Thank you for the careful and responsive work!

SEZ9

Thanks for working on this. I reviewed the latest head ebe122b23590.

What this PR fixes

User pain: the planned pip launcher (#12707) has no programmatic way to pick connectors and fetch them; users hand-edit config/plugin_config, run bin/install-plugin.sh themselves, and a typo or failed download leaves the selection half-changed with no rollback.
Fix approach: new tools/seatunnel-python-sdk/seatunnel/plugin_installer.py where select_plugins() rewrites plugin_config line by line via _CONNECTOR_LINE_RE (requested artifact IDs enabled, every other connector #-prefixed, headers/comments/line endings preserved, atomic mkstemp + os.replace), and install_plugins() validates version/layout/bash, snapshots the file bytes, calls select_plugins, runs subprocess.run([bash, script, version], cwd=seatunnel_home, check=False) with uncaptured output, and restores the snapshot on any BaseException; unit tests mock shutil.which/subprocess.run. The module has no in-tree caller and is not exported from seatunnel/__init__.py.
One-line summary: self-contained, well-tested module with a sound snapshot/restore design, but install_plugins joins script from seatunnel_home and also sets cwd=seatunnel_home (plugin_installer.py:195), so a relative home makes bash resolve the path twice and fail only after plugin_config was rewritten; the line regex is stricter than the script's first-character rule (install-plugin.sh:267), version is only emptiness-checked despite the script's own validation, the UTF-8 read can leak UnicodeDecodeError, there is no inter-process lock, and the helper is bash-only while the distribution ships install-plugin.cmd.

Runtime chain I rechecked

install_plugins(seatunnel_home, connectors, version)                       tools/seatunnel-python-sdk/seatunnel/plugin_installer.py:165
  -> version.strip() / emptiness check only                                  plugin_installer.py:165-167
  -> os.path.join(seatunnel_home, "bin", "install-plugin.sh") (relative if home is relative)  plugin_installer.py:170
  -> shutil.which("bash")                                                    plugin_installer.py:179
  -> open(plugin_config, "rb").read()  (snapshot)                            plugin_installer.py:184-185
  -> select_plugins(plugin_config, connectors)                               plugin_installer.py:191
       -> _normalize_connectors()  (_ARTIFACT_ID_RE, 'connector-' prefix)    plugin_installer.py:48-66
       -> open(..., encoding="utf-8", newline="")  (only OSError caught)     plugin_installer.py:90-96
       -> _CONNECTOR_LINE_RE.fullmatch(content) per line                     plugin_installer.py:100-110
       -> _write_atomically(path, bytes)  (mkstemp -> copymode -> os.replace)  plugin_installer.py:120-136
  -> subprocess.run([bash, script, version], cwd=seatunnel_home)             plugin_installer.py:195
       -> bin/install-plugin.sh: version case checks                         bin/install-plugin.sh:61-70
       -> while read line < config/plugin_config; first_char rule            bin/install-plugin.sh:264-267
       -> case *[!A-Za-z0-9._-]* -> exit 1                                   bin/install-plugin.sh:268-272
       -> download_release_plugin / download_plugin_with_maven               bin/install-plugin.sh:275-280
  -> on BaseException: _write_atomically(plugin_config, original) then re-raise  plugin_installer.py:204-217

Findings

Issue 1: install_plugins cannot start the script when seatunnel_home is relative: the relative script path is combined with cwd=seatunnel_home, so bash resolves it twice (dist/dist/bin/install-plugin.sh) and fails after plugin_config was already rewritten.

Location: tools/seatunnel-python-sdk/seatunnel/plugin_installer.py:195
Why it matters: - Root cause: script = os.path.join(seatunnel_home, "bin", "install-plugin.sh") (line 170) is checked with os.path.isfile(script) relative to the Python process CWD (line 172), but then passed to subprocess.run([bash, script, version], cwd=seatunnel_home, ...) (line 195). The child changes directory to seatunnel_home before exec, so bash resolves the still-relative script against the new CWD, i.e. <home>/<home>/bin/install-plugin.sh, which does not exist.
Impact: for any relative seatunnel_home (e.g. "seatunnel-dist" or "."-based paths) the pre-flight checks pass, plugin_config is rewritten by select_plugins, and bash then exits 127 with "No such file or directory". The rollback restores the file, but the user sees a confusing install failure for a path that isfile had just accepted. The cwd= is also unnecessary: install-plugin.sh derives SEATUNNEL_HOME from $0 (SEATUNNEL_HOME=$(cd "$(dirname "$0")"; cd ../; pwd)).
Fix: normalise once at the top, seatunnel_home = os.path.abspath(os.path.expanduser(seatunnel_home)), before building plugin_config/script on lines 169-170, or drop cwd=seatunnel_home. Add a test that passes os.path.relpath(self.home); the existing tests only use an absolute TemporaryDirectory and mock subprocess.run, so they cannot observe the double resolution.
Evidence: tools/seatunnel-python-sdk/seatunnel/plugin_installer.py:172 if not os.path.isfile(path):; tools/seatunnel-python-sdk/seatunnel/plugin_installer.py:195 result = subprocess.run([bash, script, version], cwd=seatunnel_home, check=False); tools/seatunnel-python-sdk/tests/test_plugin_installer.py:212 ["/usr/bin/bash", self.script_path, "3.0.1"], cwd=self.home, check=False
Suggested fix: If seatunnel_home is relative (e.g. "seatunnel-dist"), script is relative too, and because we also pass cwd=seatunnel_home, bash resolves it against the new working directory and looks for seatunnel-dist/seatunnel-dist/bin/install-plugin.sh. The isfile check on line 172 passes (it runs in the Python CWD), so plugin_config is rewritten and then the run fails with exit 127 / "No such file or directory", which the user sees as a confusing install failure. Please normalise the home first (seatunnel_home = os.path.abspath(os.path.expanduser(seatunnel_home)) before line 169), or drop cwd= since the script computes SEATUNNEL_HOME from $0 itself. A test with os.path.relpath(self.home) would lock this in; the current tests always use an absolute TemporaryDirectory.
Severity: Medium

Issue 2: select_plugins only recognises connector lines that fully match _CONNECTOR_LINE_RE; a line with stray whitespace or a trailing comment is left untouched and stays enabled, contradicting the documented "only connectors are enabled" contract and letting install-plugin.sh fail after the rewrite.

Location: tools/seatunnel-python-sdk/seatunnel/plugin_installer.py:105
Why it matters: - Root cause: plugin_installer.py:105 uses _CONNECTOR_LINE_RE.fullmatch(content) and silently appends any non-matching line unchanged (:114). bin/install-plugin.sh instead classifies by first character only (first_char != "-" && != "#" && non-empty, line 267) and then rejects the line if it contains anything outside [A-Za-z0-9._-] (lines 268-272, exit 1). So connector-kafka (trailing space), connector-kafka, or connector-kafka # keep is not a "connector line" for the Python side but is an enabled, invalid artifact for the script.
Impact: after select_plugins has rewritten the file, the script exits 1 with invalid connector artifact ID, and the rollback runs, so the user gets a script failure for something Python could have reported up front. The mirror case, #connector-kafka with a trailing space, is not recognised as a disabled connector either, so a request for kafka raises Connector(s) not found even though the connector is listed. This conflicts with the docstring (line 71) and with the pre-flight intent stated on line 177.
Fix: strip() the content before matching, and raise PluginInstallError for any line whose first non-blank character is neither # nor - yet does not match _ARTIFACT_ID_RE, so an unparseable file is rejected before the script starts.
Evidence: tools/seatunnel-python-sdk/seatunnel/plugin_installer.py:105 match = _CONNECTOR_LINE_RE.fullmatch(content); tools/seatunnel-python-sdk/seatunnel/plugin_installer.py:114 lines.append(line); tools/seatunnel-python-sdk/seatunnel/plugin_installer.py:105 match = _CONNECTOR_LINE_RE.fullmatch(content)
Suggested fix: fullmatch means a line such as connector-kafka (trailing space) or connector-kafka # note is not treated as a connector line and is appended unchanged at line 114, so it stays enabled even though the docstring promises "only connectors are enabled". bin/install-plugin.sh classifies by first character only and then rejects that same line as an invalid artifact ID, so the install fails after we already rewrote the file. The mirror case, #connector-kafka with a trailing space, yields "Connector(s) not found" for a connector that is listed. Could we strip() the content before matching, and raise PluginInstallError for any line whose first non-blank char is not #/- yet does not match _ARTIFACT_ID_RE? That keeps the "check everything before the script starts" guarantee from line 177.
Severity: Medium

Issue 3: No inter-process lock around snapshot → select → run → restore: two concurrent install_plugins calls on the same seatunnel_home interleave, so one call can install the other's selection and still return its own artifact_ids as if they were installed.

Location: tools/seatunnel-python-sdk/seatunnel/plugin_installer.py:191
Why it matters: - Root cause: the function reads original (lines 184-185), rewrites the file in select_plugins (line 191), runs the script (line 195) and restores (line 207) without any lock. bin/install-plugin.sh reads config/plugin_config only when its while read loop starts (line 264-282), so the window between the rewrite and the script reading it is open to another process.
Concrete failure: terminal A runs install_plugins(home, ["jdbc"], v); while A's bash is starting, terminal B runs install_plugins(home, ["kafka"], v). B's select_plugins comments out connector-jdbc and enables connector-kafka; A's script then reads B's selection, downloads kafka, exits 0, and A returns ["connector-jdbc"] although no jdbc jar was installed. If B then fails and restores its snapshot (which already contains A's selection), the file ends up with jdbc enabled but only kafka downloaded. The module docstring (lines 18-21) scopes the helper to a per-user cache, which does not prevent two shells of the same user from racing.
Fix: hold an exclusive lock for the whole try block (fcntl.flock on config/.plugin_config.lock, msvcrt.locking on Windows), or at least re-read the file after a zero exit and raise if the enabled set differs from artifact_ids.
Evidence: tools/seatunnel-python-sdk/seatunnel/plugin_installer.py:191 artifact_ids = select_plugins(plugin_config, connectors); tools/seatunnel-python-sdk/seatunnel/plugin_installer.py:195 result = subprocess.run([bash, script, version], cwd=seatunnel_home, check=False); tools/seatunnel-python-sdk/seatunnel/plugin_installer.py:207 _write_atomically(plugin_config, original)
Suggested fix: There is no lock around snapshot (184) → select_plugins (191) → subprocess.run (195) → restore (207). With a per-user launcher cache, two shells can run this concurrently: B's select_plugins can comment out A's connector-jdbc and enable connector-kafka before A's install-plugin.sh starts its while read loop, so A installs kafka, exits 0 and returns ["connector-jdbc"] even though no jdbc jar was downloaded. Could we hold an exclusive lock (fcntl.flock on config/.plugin_config.lock, msvcrt.locking on Windows) across the whole try block, or at least re-read the file after a zero exit and raise if the enabled set differs from artifact_ids?
Severity: Medium

Issue 4: Hard requirement on bash + bin/install-plugin.sh makes the helper unusable on Windows even though the distribution ships bin/install-plugin.cmd that reads the same config/plugin_config; worse, a WSL/Git bash.exe on PATH will be picked up and handed a Windows path.

Location: tools/seatunnel-python-sdk/seatunnel/plugin_installer.py:179
Why it matters: - Root cause: install_plugins only ever looks for bin/install-plugin.sh (plugin_installer.py:170) and refuses to run without shutil.which("bash") (plugin_installer.py:179-181). The in-tree distribution also contains bin/install-plugin.cmd, which iterates %SEATUNNEL_HOME%\config\plugin_config with the same #/- first-character rule (install-plugin.cmd:43-46) and downloads through mvnw.cmd, so the select_plugins() step is already platform-neutral; only the launch step is Unix-only.
Impact: on a Windows machine without bash the user gets a flat "bash is required" error although a working installer exists next to the script. On a machine where WSL or Git-for-Windows puts bash.exe on PATH, shutil.which returns it, the pre-flight passes, plugin_config is rewritten, and bash is handed a C:\... path it cannot open, so the call fails after the rewrite (the rollback then restores the file). The PR description reports the unit tests passing on Windows, but they mock shutil.which and subprocess.run, so this path is never exercised.
Fix: either dispatch on os.name == "nt" to ["cmd", "/c", os.path.join(seatunnel_home, "bin", "install-plugin.cmd"), version], or raise an explicit "only supported on Linux/macOS" error before touching plugin_config and document that limitation in the docstring.
Evidence: tools/seatunnel-python-sdk/seatunnel/plugin_installer.py:179 bash = shutil.which("bash"); tools/seatunnel-python-sdk/seatunnel/plugin_installer.py:181 raise PluginInstallError("bash is required to run 'bin/install-plugin.sh'.")
Suggested fix: The distribution also ships bin/install-plugin.cmd, which reads the same config/plugin_config with the same #/- first-character rule, so select_plugins() already works there; only this launch step is Unix-only. On Windows shutil.which("bash") can return WSL's bash.exe, which then gets a C:\... path it cannot open, so we fail after rewriting the file. Could we either dispatch on os.name == "nt" to cmd /c bin\install-plugin.cmd <version>, or explicitly raise "only supported on Linux/macOS" before touching plugin_config and document that?
Severity: Medium

Issue 5: version is only checked for emptiness; values that install-plugin.sh rejects, or that silently switch it into Maven mode ([, ,, +, LATEST, -SNAPSHOT), reach the script after plugin_config was rewritten, contradicting the "check everything before the script starts" comment

Location: tools/seatunnel-python-sdk/seatunnel/plugin_installer.py:166
Why it matters: - Root cause: lines 165-167 strip version and reject only the empty string; nothing else is validated even though the module already has _ARTIFACT_ID_RE ([A-Za-z0-9._-]+), which is exactly the character set bin/install-plugin.sh enforces for a release version (*[!A-Za-z0-9._-]* | "" -> invalid connector release version, exit 1, lines 68-73).
Impact: a version such as "3.0.1 beta" or "3.0/1" passes the Python pre-flight, plugin_config is rewritten, bash exits 1 and the rollback runs; that works but is exactly the "doomed install" the comment on line 177 says is avoided. More surprising: install-plugin.sh lines 61-65 switch download_method to maven for *-SNAPSHOT, LATEST, RELEASE or any version containing [, ], (, ), ,, +, which then runs ${SEATUNNEL_HOME}/mvnw dependency:get (Maven bootstrap over the network) instead of the HTTPS download, with no hint from the Python side that this will happen.
Fix: validate with the existing regex before line 169 (if not _ARTIFACT_ID_RE.fullmatch(version): raise PluginInstallError(...)), and mention the Maven fallback for snapshot/dynamic versions in the install_plugins docstring.
Evidence: tools/seatunnel-python-sdk/seatunnel/plugin_installer.py:166 if not version:; tools/seatunnel-python-sdk/seatunnel/plugin_installer.py:177 # Check everything that can fail before the script starts, so a doomed install does; tools/seatunnel-python-sdk/seatunnel/plugin_installer.py:195 result = subprocess.run([bash, script, version], cwd=seatunnel_home, check=False)
Suggested fix: bin/install-plugin.sh rejects any release version outside [A-Za-z0-9._-] (invalid connector release version, exit 1), and a version containing [, (, ,, + or equal to LATEST/RELEASE/*-SNAPSHOT silently flips it into Maven mode and runs mvnw (network bootstrap). Right now only the empty string is rejected here, so these cases reach the script after plugin_config was already rewritten, which contradicts the "check everything that can fail before the script starts" comment below. Since _ARTIFACT_ID_RE is already the script's character set, I'd validate version with it here (if not _ARTIFACT_ID_RE.fullmatch(version): raise PluginInstallError(...)) and mention the Maven fallback in the docstring.
Severity: Low

Issue 6: select_plugins/_normalize_connectors leak raw non-PluginInstallError exceptions for malformed input: a non-UTF-8 plugin_config escapes as UnicodeDecodeError, and connectors=None escapes as TypeError.

Location: tools/seatunnel-python-sdk/seatunnel/plugin_installer.py:95
Why it matters: - Root cause: the text read on line 93 uses encoding="utf-8" but only except OSError (line 95) wraps it; UnicodeDecodeError is a ValueError and propagates unwrapped. In _normalize_connectors, for name in connectors (line 56) is reached with None (the isinstance(..., str) guard on line 52 does not cover it) and raises TypeError; a non-str element is silently turned into "" (line 57) so the error message reads Invalid connector name '' and hides the offending value.
Impact: the docstrings promise :raises PluginInstallError: for unreadable files and invalid names (lines 87-88, 162-163), which is the contract the launcher will catch. These cases surface as raw tracebacks instead. The file is not modified in either case, and inside install_plugins the UnicodeDecodeError is raised before the try on line 193, so no rollback is involved; the gap is purely in the error contract.
Fix: except (OSError, UnicodeDecodeError) as exc: on line 95; in _normalize_connectors, reject None/non-iterables explicitly and keep the original repr of a non-str element in the message.
Evidence: tools/seatunnel-python-sdk/seatunnel/plugin_installer.py:95 except OSError as exc:; tools/seatunnel-python-sdk/seatunnel/plugin_installer.py:56 for name in connectors:; tools/seatunnel-python-sdk/seatunnel/plugin_installer.py:57 name = name.strip() if isinstance(name, str) else ""
Suggested fix: UnicodeDecodeError is a ValueError, not an OSError, so a plugin_config with a stray non-UTF-8 byte escapes here as a raw traceback instead of the documented PluginInstallError. except (OSError, UnicodeDecodeError) as exc: keeps the contract. Same spirit in _normalize_connectors: connectors=None raises TypeError on line 56, and a non-str element is reported as Invalid connector name '' (line 57 drops the value) — worth guarding and keeping the original repr in the message.
Severity: Low

Issue 7: The rollback writes original unconditionally, even when select_plugins did not change the file; this turns a plain script failure into a double-failure in a directory without write permission and churns inode/ownership/symlink of a file that was never modified.

Location: tools/seatunnel-python-sdk/seatunnel/plugin_installer.py:207
Why it matters: - Root cause: _write_atomically(plugin_config, original) on line 207 runs for every failure, but select_plugins deliberately skips the write when updated == original (line 123). In that idempotent case the restore is the only write the call performs.
Impact: when config/ is not writable (e.g. a read-only cache mount) but the selection was already correct, the script's real failure (network, checksum) is reported as ... Restoring the original plugin config also failed: Cannot write plugin config ..., which points the user at a file problem that does not exist. In the normal case, mkstemp + shutil.copymode + os.replace (lines 132-137) replaces the inode: a symlinked plugin_config becomes a regular file and ownership flips to the calling user, even though the content never changed.
Fix: have select_plugins report whether it actually wrote (or compare the current bytes with original in the except block) and only restore when the file differs.
Evidence: tools/seatunnel-python-sdk/seatunnel/plugin_installer.py:123 if updated != original:; tools/seatunnel-python-sdk/seatunnel/plugin_installer.py:136 shutil.copymode(path, temp_path); tools/seatunnel-python-sdk/seatunnel/plugin_installer.py:137 os.replace(temp_path, path)
Suggested fix: This restore runs even when select_plugins skipped its write because the selection was already correct (line 123). In that case it is the only write of the whole call: in a non-writable config/ a network/checksum failure in the script is reported as a restore failure too, and in the normal case we needlessly replace the inode (symlink → regular file, owner change). Could select_plugins report whether it actually wrote, and the restore only run when it did (or when the current bytes differ from original)?
Severity: Low

Issue 8: The example version in the error message, '3.0.1', is the version the PR description itself reports as returning 404 on Maven Central; the user-facing hint should point to a published release or avoid a literal.

Location: tools/seatunnel-python-sdk/seatunnel/plugin_installer.py:167
Why it matters: - Root cause: plugin_installer.py:167 hard-codes e.g. '3.0.1' into the PluginInstallError text. The PR description notes that only 3.0.0 is currently published and that 3.0.1 fails with 404 in the author's own manual test.
Impact: a launcher user who hits the empty-version error and copies the suggested literal will immediately hit a second failure from install-plugin.sh, which is a poor first experience for the pip launcher. It also bakes a release number into library code that will go stale with every release, while the docstring on line 160 already states the real rule ("it should match the engine version").
Fix: phrase the hint without a literal ("A connector version is required; it should match your SeaTunnel engine version.") or let the launcher pass its own version into the message.
Suggested fix: Nit: the PR description mentions that 3.0.1 currently 404s on Maven Central (only 3.0.0 is published), so a user who copies this example straight into the call will hit a second failure from the script. Could we phrase the hint without a literal release number ("it should match your SeaTunnel engine version") or have the launcher supply its own version in the message? That also avoids this string going stale with each release.
Severity: Low

Review conclusion

Conclusion: final approval remains pending on the previous comments

2. Remaining follow-ups

Issue 1: install_plugins cannot start the script when seatunnel_home is relative: the relative script path is combined with cwd=seatunnel_home, so bash resolves it twice (dist/dist/bin/install-plugin.sh) and fails after plugin_config was already rewritten.
Issue 2: select_plugins only recognises connector lines that fully match _CONNECTOR_LINE_RE; a line with stray whitespace or a trailing comment is left untouched and stays enabled, contradicting the documented "only connectors are enabled" contract and letting install-plugin.sh fail after the rewrite.
Issue 3: No inter-process lock around snapshot → select → run → restore: two concurrent install_plugins calls on the same seatunnel_home interleave, so one call can install the other's selection and still return its own artifact_ids as if they were installed.
Issue 4: Hard requirement on bash + bin/install-plugin.sh makes the helper unusable on Windows even though the distribution ships bin/install-plugin.cmd that reads the same config/plugin_config; worse, a WSL/Git bash.exe on PATH will be picked up and handed a Windows path.
Issue 5: version is only checked for emptiness; values that install-plugin.sh rejects, or that silently switch it into Maven mode ([, ,, +, LATEST, -SNAPSHOT), reach the script after plugin_config was rewritten, contradicting the "check everything before the script starts" comment
Issue 6: select_plugins/_normalize_connectors leak raw non-PluginInstallError exceptions for malformed input: a non-UTF-8 plugin_config escapes as UnicodeDecodeError, and connectors=None escapes as TypeError.
Issue 7: The rollback writes original unconditionally, even when select_plugins did not change the file; this turns a plain script failure into a double-failure in a directory without write permission and churns inode/ownership/symlink of a file that was never modified.
Issue 8: The example version in the error message, '3.0.1', is the version the PR description itself reports as returning 404 on Maven Central; the user-facing hint should point to a published release or avoid a literal.

Thanks for the updates so far. Happy to discuss the remaining points.

karthik-0306 · 2026-10-26T06:48:26Z

Thanks a lot @SEZ9 for the thorough review. All 8 points are addressed in the latest commit:

Relative seatunnel_home: it's now resolved with os.path.abspath(os.path.expanduser(...)) before building any path, so bash always gets an absolute script path. Test: test_relative_home_is_resolved_to_an_absolute_script_path. I also ran it on Linux against the real install-plugin.sh with a relative home, and connector-assert-3.0.0.jar was installed.
Line parsing: lines are now classified exactly like install-plugin.sh (first character: blank / - / # skipped, anything else is a connector). Whitespace around a connector ID is tolerated (#connector-kafka is recognized as disabled), and any line the script would reject (connector-kafka # keep, whitespace-only, an indented comment) raises PluginInstallError with its line number before anything is rewritten.
Concurrency: install_plugins() now holds an exclusive fcntl.flock on config/.plugin_config.lock across snapshot → select → run → restore. test_lock_is_held_while_the_script_runs checks that another process can't take the lock during the run (runs on Linux/macOS). I also ran two real processes against one copy on Linux: they were serialized, and each script saw its own selection.
Windows: I went with your second option. install_plugins() now raises a clear "Linux and macOS only" error before touching plugin_config or looking for bash, and the docstrings say so. I didn't dispatch to install-plugin.cmd because it never checks errorlevel or exits non-zero, so a failed mvnw.cmd download would look like success and the rollback couldn't work. select_plugins() itself stays cross-platform.
Version: validated with the script's character set before any rewrite, and the docstring now mentions the Maven fallback for LATEST / RELEASE / *-SNAPSHOT.
Error contract: UnicodeDecodeError is wrapped in PluginInstallError, and connectors=None / non-string names raise PluginInstallError showing the original value.
Restore only when changed: the rollback now compares the current bytes with the snapshot and only writes if they differ (test_failed_install_does_not_rewrite_an_unchanged_file).
Version hint: no literal release in the message anymore ("it should match your SeaTunnel engine version").

10 new tests (37 in this module). All 65 SDK tests pass on Windows (Python 3.9 / 3.11 / 3.13; the lock test is skipped there, since there's no fcntl) and on Linux (Python 3.10, with the lock test running).

DanielLeens

Thank you, @karthik-0306, for the quick and thorough follow-up. I re-reviewed the full diff at the new head 048bb584a4518027e7b738c3f556f4a0ddf73baf (commit "Address review: absolute home, script-compatible parsing, lock, version check"). It addresses all 8 points that @SEZ9 raised on ebe122b23, and I checked each one against the code and against bin/install-plugin.sh / bin/install-plugin.cmd. This is a read-only static review: I did not build, run tests or execute anything locally; CI is the only runtime evidence.

What Problem Does This PR Solve?

User pain point: callers of the Python SDK (and the pip launcher in #12707) have no safe way to select connectors in config/plugin_config and run bin/install-plugin.sh; doing it by hand leaves the selection half-changed when a download fails.
Fix approach: select_plugins() rewrites plugin_config atomically (disable with a leading #, never delete lines); install_plugins() validates everything first, takes an exclusive lock, runs the script, and restores the original bytes on failure or interrupt.
One-sentence summary: a small, self-contained SDK helper that wraps the existing installer with rollback, and it now also matches the script's parsing and validation rules.

1. Code Change Review

1.1 Core Logic Analysis

Only tools/seatunnel-python-sdk/seatunnel/plugin_installer.py and tools/seatunnel-python-sdk/tests/test_plugin_installer.py change (new files, no Java/engine/core/API code).

Runtime path of install_plugins on this head:

install_plugins(home, connectors, version)                          plugin_installer.py:230
  -> reject Windows (sys.platform == "win32")                         :262
  -> version: strip, non-empty, [A-Za-z0-9._-]+                       :268-277
  -> home = abspath(expanduser(home)); check config/plugin_config
     and bin/install-plugin.sh exist                                  :280-288
  -> shutil.which("bash")                                             :290
  -> with _exclusive_lock(config/.plugin_config.lock)  (fcntl.flock)  :294
       -> snapshot original bytes                                     :296
       -> select_plugins(): normalize names, classify lines like the
          script, raise before any write on an unknown / invalid line,
          atomic write only if changed                                :303 -> :91-170
       -> subprocess.run([bash, abs_script, version], cwd=home)       :307
            exit != 0 / OSError / KeyboardInterrupt / SystemExit
              -> _restore_if_changed(): write back only if bytes differ  :321
              -> restore failure: Exception -> one PluginInstallError with both messages;
                 KeyboardInterrupt/SystemExit re-raised unchanged     :322-331
  -> lock released when the descriptor is closed                      :227

Verification of the 8 earlier points (all closed):

Relative home: resolved once at :280 before any path is built, so bash gets an absolute script path. Test test_relative_home_is_resolved_to_an_absolute_script_path.
Line parsing: select_plugins now uses the script's first-character rule (:131-153; script install-plugin.sh:264-272). A line the script would reject raises PluginInstallError with its line number before anything is written.
Concurrency: fcntl.flock around snapshot -> select -> run -> restore (:206-227, :294). The lock file is intentionally left in place and the reason is documented.
Windows: explicit "Linux and macOS only" error before touching the file or looking for bash (:262-266). I confirmed your reasoning for not dispatching to the .cmd installer: bin/install-plugin.cmd contains no errorlevel check and no exit, so a failed download would look like success and rollback could never trigger. This is exactly the second option @SEZ9 offered.
Version: validated with the script's character set before any rewrite (:273), and the docstring now documents the Maven fallback (:252-254).
Error contract: UnicodeDecodeError is wrapped (:120), and None / non-string connector arguments raise PluginInstallError showing the original value (:65-80).
Restore only when changed: _restore_if_changed (:191-199), test test_failed_install_does_not_rewrite_an_unchanged_file.
The version hint no longer contains a literal release number (:271).

No source-level blocker found. Two optional hardening notes are in 1.4.

1.2 Compatibility Impact

Fully compatible: new Python module and tests only; no Java, engine, config option, protocol or persisted-state change. Python 3.9 stays supported (add_note is guarded with hasattr; fcntl is imported lazily and only on the POSIX path).

1.3 Performance / Side-Effect Analysis

Negligible. Side effects are the plugin_config rewrite (restored on failure) and a small config/.plugin_config.lock file created inside the distribution copy. The blocking flock has no timeout, which is acceptable because the lock is only contended by another install_plugins call on the same copy, and the docs scope the helper to a caller-owned cache directory.

1.4 Error Handling and Logging

Original errors are chained, restore failures are reported together with the original failure, and interrupts propagate. No secrets are logged. No blocking issues. Optional hardening:

Issue 1 (optional, Low, carryover): a small interrupt window between the rewrite and the rollback guard

Location: tools/seatunnel-python-sdk/seatunnel/plugin_installer.py:303-305
Problem: select_plugins(...) is called before the try that owns the rollback. If a KeyboardInterrupt lands after os.replace inside select_plugins but before the try starts, the new selection stays on disk. Sorry for missing this in the earlier rounds; this window already existed in the previously reviewed heads.
Potential risk: very small (a few bytecodes wide), and the next call starts from a valid file, so impact is limited to one leftover selection.
Best improvement: move the select_plugins(...) call inside the existing try (the original snapshot is already taken before it), so the same BaseException handler covers it.
Severity: Low
Raised by another reviewer: No

Notes, no action required: (a) the lock file is created with O_CREAT, so a distribution whose config/ is read-only now fails at lock creation with a clear PluginInstallError; this is consistent with the helper's ownership contract, which already requires a writable copy. (b) The version check is stricter than the script (Maven range syntax such as [3.0,) is rejected rather than routed to Maven), which is a safe, documented choice.

2. Code Quality Assessment

2.1 Coding Standards

ASF headers are present; public functions and non-obvious helpers have docstrings that explain why (ownership boundary, LF requirement, lock-file lifetime, interrupt handling). Section comments on the ordering of checks are clear.

2.2 Test Coverage and Test Stability

Rating: Stable. 37 tests in the module cover selection and idempotency, whitespace handling, rejected lines, non-UTF-8 and invalid arguments, script call contract, rollback for exit code / OSError / KeyboardInterrupt, double failure, pre-flight ordering, relative home, Windows rejection, no-op restore, and the lock held during the run. They use temporary directories and mocks only: no sleeps, no network, no fixed ports, no shared mutable state. They have failure power: for example, removing the _restore_if_changed comparison turns test_failed_install_does_not_rewrite_an_unchanged_file red, and removing the lock turns test_lock_is_held_while_the_script_runs red. The lock test is skipped where fcntl is unavailable (Windows) and the lock is replaced by a null context there, which is clearly commented. The existing SDK tests were not weakened.

2.3 Documentation Updates

Docstring-level documentation is sufficient for this internal SDK helper (platform limit, lock, version rules, ownership boundary). User-facing docs belong with the launcher work in #12707, as agreed earlier.

3. Architectural Soundness

3.1 Elegance: it wraps the existing script instead of reimplementing download and checksum logic, and now mirrors the script's own line-classification rule.
3.2 Maintainability: clear boundaries, no coupling to engine/core.
3.3 Extensibility: a good base for the launcher; Windows support can be added later if install-plugin.cmd ever reports failures through its exit code.
3.4 Historical-version compatibility: not applicable (new module, no persisted state or protocol).

4. Issue Summary

Number	Issue	Location	Severity
1	Optional: move `select_plugins` inside the rollback `try` to close a tiny interrupt window (carryover)	`tools/seatunnel-python-sdk/seatunnel/plugin_installer.py:303`	Low

5. Merge Recommendation

CI facts for head 048bb584a4: the fork Build run 40281532333 is queued at the time of this review, and no apache-side check runs exist yet for this SHA (the SeaTunnel Python SDK workflow needs a maintainer to approve it, as before). The PR is diverged (ahead 5, behind 21) with no merge conflict; being behind alone does not need a sync.

Conclusion: Ready to merge

Blockers - must be fixed
- None on the code side. Remaining gates are CI only: a green fork Build on this head, and a write-access maintainer approving the apache-side SeaTunnel Python SDK workflow and the merge. If an unrelated Zeta engine job fails again as before, please re-run only that job.
Recommended fixes - non-blocking
- Issue 1 above (optional).

Overall assessment: all of @SEZ9's follow-ups are resolved with matching tests, and I found no new blocking problem. I am happy to re-confirm once Build is green on this head. Thank you for the careful and responsive work!

Better alternative: none needed; the chosen design (wrap the script, lock, snapshot/restore) is the right size for this helper.

karthik-0306 · 2026-10-26T16:45:39Z

Thanks @DanielLeens. I took the optional Issue 1 in the latest commit: select_plugins(...) now runs inside the rollback try, so an interrupt right after the rewrite is rolled back too (test_interrupt_right_after_the_rewrite_is_rolled_back, which fails on the previous head). While re-reading the module line by line I also closed the same error-contract gap @SEZ9 pointed out for connectors=None, but for seatunnel_home: None, a number or a bytes path now raise PluginInstallError instead of a raw TypeError.

All 67 SDK tests pass on Linux with Python 3.9, 3.10 and 3.13 (lock test included), and on Windows with 3.9 / 3.11 / 3.13 (lock test skipped, no fcntl).

About the red Test with Python 3.9 check: it fails at "Set up Python" before any test runs, because ubuntu-latest is now Ubuntu 26.04 and setup-python has no 3.9 build for it. That affects every SDK PR, so I've opened #12994 to pin the SDK workflow to ubuntu-24.04 instead of mixing it into this PR.

DanielLeens

Thank you, @karthik-0306, for the reply and for taking the optional carryover from my last round so quickly. I re-reviewed the full diff at the latest head 5c6b35f833220a2984b80913c2cd739e90d08412 ("Roll back an interrupt right after the rewrite and ..."), and I also re-read the points @SEZ9 raised on ebe122b23 against the current code. This is a read-only static review: I did not build or run anything locally, and CI is the only runtime evidence.

What Problem Does This PR Solve?

User pain point: Python callers (and the pip launcher planned in #12707) have no safe way to select connectors in config/plugin_config and run bin/install-plugin.sh; doing it by hand can leave the selection half-changed when a download fails.
Fix approach: select_plugins() rewrites plugin_config atomically (unrequested connectors are disabled with #, never deleted); install_plugins() validates everything first, takes an exclusive lock, runs the script, and restores the original bytes on failure or interrupt.
One-sentence summary: a small, self-contained SDK helper that wraps the existing installer with rollback, locking and script-compatible parsing.

1. Code Change Review

1.1 Core Logic Analysis

Only two new files change: tools/seatunnel-python-sdk/seatunnel/plugin_installer.py and tools/seatunnel-python-sdk/tests/test_plugin_installer.py. There is no Java, engine, core, API, checkpoint or serialization code in the diff, and the module is not yet called from anywhere in-tree, so the "normal path" is the future launcher calling install_plugins().

Delta since my last review (head 048bb584a): the select_plugins(...) call moved inside the rollback try, and seatunnel_home is now type-checked (None, numbers and bytes raise PluginInstallError).

Before (previous head):

artifact_ids = select_plugins(plugin_config, connectors)   # outside the guard
try:
    result = subprocess.run(...)

After (this head):

try:
    # Inside the try, so an interrupt right after the rewrite is rolled back too.
    artifact_ids = select_plugins(plugin_config, connectors)
    try:
        result = subprocess.run([bash, script, version], cwd=seatunnel_home, check=False)
    ...
except BaseException as exc:
    _restore_if_changed(plugin_config, original)

Runtime path of install_plugins on this head:

install_plugins(home, connectors, version)
  -> reject Windows; validate version with [A-Za-z0-9._-]+      (plugin_installer.py, pre-flight block)
  -> type-check home, abspath(expanduser(home)), check config/plugin_config and bin/install-plugin.sh exist
  -> shutil.which("bash")
  -> with _exclusive_lock(config/.plugin_config.lock)            (fcntl.flock)
       -> snapshot original bytes
       -> try:
            select_plugins(): normalize names, classify lines like the script,
                              raise before any write on a rejected line, atomic write only if changed
            subprocess.run([bash, <abs script>, version], cwd=home)
          except BaseException:
            _restore_if_changed(): write back only if bytes differ
            restore failure: Exception -> one PluginInstallError naming both;
                             KeyboardInterrupt / SystemExit re-raised as they are

Correctness checks I made:

Moving select_plugins into the try is safe. If select_plugins raises before writing (unknown connector, rejected line, unreadable file), _restore_if_changed reads the file, sees identical bytes and writes nothing, so the "file is not modified on validation errors" contract holds (and the new test_interrupt_right_after_the_rewrite_is_rolled_back covers the case where the write did happen).
The script reads lines with IFS= read -r and rejects anything outside [A-Za-z0-9._-] (bin/install-plugin.sh:264-272). Because select_plugins strips the line before rewriting and rejects whitespace-only or invalid IDs up front, the Python side is never more permissive than the script for the lines it keeps enabled.
The isinstance(os.fspath(...), str) check short-circuits correctly, so None and 42 fail on the first clause and bytes paths fail on the second; no raw TypeError escapes.
Earlier points from the previous rounds (relative home, line parsing, lock, Windows, version validation, error contract, restore-only-if-changed, version hint) remain closed; I re-confirmed them in the current code.

No new defect found in this delta.

1.2 Compatibility Impact

Fully compatible. New Python module and tests only; no Java, engine, config option, protocol or persisted-state change. Python 3.9 stays supported (add_note is guarded with hasattr; fcntl is imported lazily and only on the POSIX path).

1.3 Performance / Side-Effect Analysis

Negligible. The side effects are the in-place plugin_config rewrite (restored on failure) and a small config/.plugin_config.lock file left in the caller-owned distribution copy, which the docstring explains. The blocking flock has no timeout, which is acceptable because it is only contended by another install_plugins call on the same copy, and the ownership boundary is documented.

1.4 Error Handling and Logging

Original errors are chained, restore failures are reported together with the original failure, and interrupts propagate unchanged. Nothing sensitive is logged. No formal issues. Two optional observations that I am deliberately not raising as issues:

A non-string version reports "A connector version is required" rather than a type message; the behavior is safe and tiny.
Replacing plugin_config via os.replace turns a symlinked file into a regular file when a rewrite is needed; this is consistent with the documented "caller-owned copy" contract.

2. Code Quality

2.1 Coding Standards

ASF license headers are present, public functions and non-obvious helpers have docstrings that explain why (ownership boundary, LF requirement, lock-file lifetime, interrupt handling), and the new inline comment on the try placement states the reason. No missing comments on new non-trivial methods.

2.2 Test Coverage and Test Stability

Rating: Stable. The tests use temporary directories and mocks only, with no sleeps, network or fixed ports. The new test_interrupt_right_after_the_rewrite_is_rolled_back has failure power: it calls the real select_plugins, then raises KeyboardInterrupt, and asserts the file is byte-identical and subprocess.run was never called; moving the call back outside the try makes it red. test_invalid_seatunnel_home_raises_plugin_install_error covers None, 42 and bytes and asserts the file is untouched. The lock test is skipped where fcntl is unavailable (Windows), which is commented. No existing test was weakened.

CI facts for head 5c6b35f83: the fork Build run 40359361887 is completed with success. The apache-side check list is empty for this SHA (status pending, no check runs), which is consistent with the SeaTunnel Python SDK workflow still waiting for a maintainer to approve it. The author reports that a red Test with Python 3.9 check comes from setup-python having no 3.9 build for the new ubuntu-latest image and that this is handled in #12994; I did not verify that independently, and it is not caused by this diff.

2.3 Documentation

Docstring-level documentation is sufficient for this internal helper. User-facing docs (docs/en and docs/zh) are not required now; they belong with the launcher work in #12707, as agreed earlier.

3. Architectural Soundness

3.1 Precise fix / Temporary workaround / Long-term solution

Precise fix and a sound long-term shape for this helper: it wraps the existing script instead of duplicating download and checksum logic, and it mirrors the script's own line-classification rule.

3.2 Maintainability

Clear boundaries, no coupling to engine or core, and the transactional logic (snapshot, select, run, restore) lives in one function with a single exception handler.

3.3 Extensibility

Good base for the launcher. Windows can be added later if install-plugin.cmd ever reports download failures through its exit code.

3.4 Historical-version compatibility

Not applicable: new module, no persisted state, protocol or config schema.

4. Issue Summary

Number	Issue	Location	Severity
-	No open issues on this head	-	-

5. Merge Recommendation

Conclusion: Ready to merge

Blockers - must be fixed
- None on the code side. The remaining gates are process only: a write-access maintainer approving the apache-side SeaTunnel Python SDK workflow run and the merge itself.
Non-blocking fixes
- None required.

Overall assessment: the latest commit closes the interrupt window I flagged as a carryover (sorry again for not catching it in the first round), the added tests would fail without the change, and I found no new problem. Thank you for the careful and responsive work on this.

Better alternative: none needed; wrap the script, lock, snapshot and restore is the right size for this helper.

SEZ9 · 2026-10-27T02:28:45Z

Thanks @karthik-0306. Moving select_plugins(...) inside the rollback try so an interrupt right after the rewrite is rolled back too is a good improvement, and having a regression test for it is welcome. Raising PluginInstallError for a None, numeric or bytes seatunnel_home is in line with the error-contract point from F6.

A few things I still need to verify in the diff before closing the earlier findings:

F3 (inter-process lock) and F4 (Windows): the lock test being skipped on Windows because fcntl is unavailable suggests the lock is POSIX-only. Could you point me to how concurrent install_plugins calls and the Windows path (bin/install-plugin.cmd) are handled now?
F1, F2, F5, F7, F8: could you point me to the specific changes or tests that address each, or say which ones are intentionally deferred? I'll check them against the diff rather than closing them on description alone.

On the red Test with Python 3.9 check: if it fails at "Set up Python" because of the runner image, handling the runner pin in a separate PR makes sense. I'd still like to see this PR's CI green (after rebasing on that fix) before merging.

DanielLeens · 2026-10-28T10:42:10Z

Thanks @SEZ9, and thanks @karthik-0306. I looked at the two open questions against the current head (5c6b35f83) so the next pass is quicker.

F3 (concurrency) and F4 (Windows). Both are handled in plugin_installer.py. install_plugins() takes an exclusive fcntl.flock on config/.plugin_config.lock (_exclusive_lock, around line 207) for the whole snapshot, select, run and restore sequence. This is POSIX-only on purpose, and install_plugins() rejects Windows up front (_is_windows() check, around line 262) with a clear "Linux and macOS only" error before it touches plugin_config. That is why the lock test is skipped on Windows: the code path it exercises is never reachable there. The author's reason for not dispatching to bin/install-plugin.cmd is that the script does not propagate a failed download as a non-zero exit, so the rollback could not be trusted. That is a sound trade-off in my view.

F1, F2, F5, F7, F8. The author's comment from 2026-10-26 maps these one by one to code and tests (absolute seatunnel_home, line classification identical to install-plugin.sh, version validation, restore only when the bytes changed, no hard-coded release in the hint). Karthik, it would help if you could also paste the test name next to each F-number in a reply to SEZ9's comment, so the review can close them without re-deriving.

CI. The red Test with Python 3.9 check is the runner-image problem that is already fixed on dev by #12994 (pin to ubuntu-24.04). The minimum-cost next step is to rebase this branch onto the latest dev and let the check re-run; no extra change is needed in this PR. I have not changed my view of the code, and I will re-review as soon as a new commit lands.

…s on failed restore

…rsing, lock, version check

…validate seatunnel_home

karthik-0306 · 2026-10-28T18:25:48Z

Thanks @SEZ9, and thanks @DanielLeens for the summary. I've rebased onto the latest dev, which includes #12994 (SDK workflow pinned to ubuntu-24.04), so Test with Python 3.9 can run now. No code changed in the rebase. Here is each finding mapped to the code (line numbers in tools/seatunnel-python-sdk/seatunnel/plugin_installer.py) and its test in tests/test_plugin_installer.py. Nothing is deferred:

Finding	Where	Test(s)
F1 relative `seatunnel_home`	`:286` resolves `os.path.abspath(os.path.expanduser(...))` before any path is built; `:279-284` rejects a non-path home	`test_relative_home_is_resolved_to_an_absolute_script_path`, `test_invalid_seatunnel_home_raises_plugin_install_error`
F2 line parsing	`:133-153`: same first-character rule as `install-plugin.sh` (blank / `-` / `#` skipped, anything else is a connector); whitespace tolerated (`:138`), a line the script would reject raises with its line number before any write (`:148-153`)	`test_connector_lines_with_stray_whitespace_are_recognized`, `test_disabled_connector_with_trailing_whitespace_can_be_enabled`, `test_line_rejected_by_install_script_raises_and_leaves_file_unchanged`
F3 inter-process lock	`_exclusive_lock` (`:207-227`) takes `fcntl.flock(LOCK_EX)` on `config/.plugin_config.lock`; `install_plugins` holds it for the whole snapshot → select → run → restore (`:300`)	`test_lock_is_held_while_the_script_runs` (another process cannot take the lock during the run; runs on Linux/macOS, including the Ubuntu CI job)
F4 Windows	`install_plugins` rejects Windows first (`:262`), before looking for bash or touching `plugin_config`, so the POSIX-only lock is never reached there. Not dispatched to `bin/install-plugin.cmd`: it has no `errorlevel` check and no `exit`, so a failed `mvnw.cmd` download returns success and the rollback could not trigger. `select_plugins()` stays cross-platform	`test_windows_is_rejected_before_rewriting`
F5 version	`:273` validates with the script's character set before any rewrite; Maven fallback for `LATEST` / `RELEASE` / `*-SNAPSHOT` documented in the docstring	`test_invalid_version_raises_before_rewriting`, `test_empty_version_raises`
F6 error contract	`UnicodeDecodeError` wrapped (`:120`); `None` / non-iterable (`:71`) and non-string names (`:77`) raise `PluginInstallError` with the original value	`test_non_utf8_file_raises_plugin_install_error`, `test_invalid_connectors_argument_raises_plugin_install_error`
F7 restore only when changed	`_restore_if_changed` (`:191-199`) writes only if the current bytes differ from the snapshot; used at `:327`	`test_failed_install_does_not_rewrite_an_unchanged_file`
F8 version hint	no literal release in the messages anymore (`:271`, `:276`: "it should match your SeaTunnel engine version")	`test_empty_version_raises` (asserts the message has no `3.0.1`)

On concurrency, beyond the unit test: I also ran two real install_plugins processes against one copy on Linux. They were serialized, and each install-plugin.sh run saw only its own selection. All 67 SDK tests pass on Linux with Python 3.9, 3.10 and 3.13 (lock test included).

SEZ9 · 2026-10-29T02:14:38Z

Thanks @karthik-0306 for the detailed mapping — that makes the re-review much easier.

A few notes before I go through the diff:

F1–F6: the approach you describe for each (resolving seatunnel_home to an absolute path up front, mirroring the script's first-character rule and failing with a line number before any write, holding _exclusive_lock across snapshot → select → run → restore, rejecting Windows before probing for bash, validating version against the script's character set, and wrapping UnicodeDecodeError / None / non-string names into PluginInstallError) matches what was asked. I'll verify against the code and the listed tests and update the findings accordingly.
F4: your reasoning for not dispatching to bin/install-plugin.cmd (no errorlevel check / no exit, so a failed mvnw.cmd download would look like success and defeat the rollback) makes sense to me. It would help Windows users if that rationale were visible in the docstring or the error message.
F7 / F8: your comment appears cut off partway through the F6 row on my end, so I can't see the rows for the unconditional rollback write when select_plugins made no change, or the '3.0.1' literal in the error-message hint. Could you repost or point me to where those are handled (and their tests)?

Once I have that I'll do a final pass over the diff.

karthik-0306 · 2026-10-29T10:01:06Z

Thanks @SEZ9. Sorry, the table was cut off. Here are the remaining rows, plus the F4 pointer.

F4 rationale: it is in the install_plugins docstring (plugin_installer.py L233-235): bin/install-plugin.cmd does not report download failures through its exit code, so it cannot be used with the rollback, and a WSL or Git bash cannot run the script from a Windows path. The error raised on Windows says that install_plugins() runs bin/install-plugin.sh with bash and is supported on Linux and macOS only, and that select_plugins() works on any platform. Test: test_windows_is_rejected_before_rewriting.

#	Finding	Fix	Code	Tests
F7	Rollback wrote `plugin_config` back even when `select_plugins` had changed nothing	The rollback goes through `_restore_if_changed`, which reads the current bytes and writes the snapshot back only if they differ. A failed install on an already-correct config no longer touches the file, so its mtime and inode stay the same.	`_restore_if_changed` (L191-199), called from the `except BaseException` handler in `install_plugins` (L327)	`test_failed_install_does_not_rewrite_an_unchanged_file`, plus `test_nonzero_exit_raises_and_restores_plugin_config` and `test_failed_install_restores_previous_selection_and_line_endings` for the case where a restore is needed
F8	The empty-version error hinted at a hard-coded `3.0.1`, which is not a published version	The hint no longer names any version. It says: "A connector version is required; it should match your SeaTunnel engine version." The invalid-version message ends with the same sentence.	`install_plugins` (L268-277)	`test_empty_version_raises` asserts that "engine version" is in the message and "3.0.1" is not. `test_invalid_version_raises_before_rewriting` covers the invalid-character case.

No code changed since the last push (head 1c42f78cb), and the checks are rerunning on the rebased branch. Ready for your final pass whenever you are.

DanielLeens · 2026-10-29T17:13:24Z

Thanks @karthik-0306 and @SEZ9. The full table and final fragments are readable; no repost is needed. On the current head, plugin_installer.py:191-199 only restores changed bytes, :233-235 already explains the Windows launcher limitation, and :268-277 has the version-independent engine-version hint. The existing tests at test_plugin_installer.py:437,475,486 cover those contracts. My exact-head source review already cleared the implementation. The earlier Python-3.9 runner-image advice refers to the pre-rebase state; the present Build is green. That is separate from any repository-required SDK workflow approval, and I do not claim to have run the author's reported local Python matrix.

Checked 2026-10-29T17:13:19+00:00; head 1c42f78cbda1242d35ee6e546d993b4017b30a36. Signal pull request update = SUCCESS; Build = SUCCESS

Comparison: base dev, head 1c42f78cbda1242d35ee6e546d993b4017b30a36, diverged, ahead 6 / behind 6; GitHub mergeable state blocked. Behind/diverged alone is recorded metadata and does not add a sync request.

Daniel has READ repository permission. This is a discussion/CI clarification, not a new complete source review, an effective APPROVED event or dismissal of historical requested changes. A write-capable maintainer must perform any required final approval/dismissal. Thanks for keeping the follow-up focused.

SEZ9 · 2026-10-30T02:27:33Z

Thanks for the follow-up, @karthik-0306.

F4 — the rationale you describe (a bin/install-plugin.cmd that can't take part in the rollback, and a WSL/Git bash being handed a Windows path) sounds reasonable, and an explicit error that points to select_plugins() as the cross-platform alternative is the behaviour I'd expect. I can't confirm the docstring, the error message or test_windows_is_rejected_before_rewriting from this thread alone, so I'll verify against the diff. One small ask meanwhile: please keep the docstring text and the raised error message saying the same thing (Linux/macOS only, select_plugins() anywhere) so they don't drift.

F7 — comparing the current bytes against the snapshot and only writing when they differ is what I was after, and test_failed_install_does_not_rewrite_an_unchanged_file sounds like the right case to cover. Same caveat: I'll mark it resolved once I've seen _restore_if_changed in the diff.

The table got cut off again, this time at test_nonzero_e in the F7 tests column. Could you repost the rest? I still need the Fix / Code / Tests rows for F1 (relative seatunnel_home + cwd double-resolution), F2 (connector lines with stray whitespace or trailing comments), F3 (inter-process lock), F5 (version validation before the script starts), F6 (UnicodeDecodeError / TypeError leaking from select_plugins / _normalize_connectors) and F8 (the '3.0.1' example in the error hint). Posting it as a code block or a short bulleted list might avoid the truncation.

Once I have those I'll go through everything in one pass.

DanielLeens · 2026-10-30T03:26:54Z

Thanks @SEZ9. The complete Fix/Code/Tests replies are readable through the API; no repost is needed for my review. The current head already has _restore_if_changed at plugin_installer.py:191-199, Linux/macOS-only wording at :233-235 with the matching error, and engine-version-independent guidance at :268-277. The existing regression tests for unchanged-file restoration, Windows rejection and the version hint were included in the earlier exact-head recheck. My source conclusion remains clear. Build plus Python 3.9 and 3.13 checks are now SUCCESS, so the earlier Python-runner/workflow-approval guidance is superseded by those actual results. The remaining gate is effective approval by a write-capable maintainer; no additional installer patch or full-table repost is requested.

Snapshot 2026-10-30T03:23:54.152060+00:00: head 1c42f78cbda1242d35ee6e546d993b4017b30a36; Signal pull request update = SUCCESS; Test with Python 3.9 = SUCCESS; Test with Python 3.13 = SUCCESS; Build = SUCCESS.

Base dev; comparison diverged, ahead 6, behind 9; merge state blocked. Behind/diverged alone is recorded metadata and does not add a sync request. This is an unchanged-head discussion/CI clarification, not a new complete source review, an effective approval or a dismissal of historical requested changes.

karthik-0306 · 2026-10-30T09:42:56Z

Thanks @SEZ9. Here is the full mapping as a list, in case the table was cut off on your side.

On the docstring/error wording: both already say Linux/macOS only. The error also points to select_plugins() as the cross-platform alternative, and the docstring (L233-235) adds why bin/install-plugin.cmd can't be used. I'm happy to repeat the select_plugins() clause in the docstring if you'd like it there. I held off on pushing so the head you and @DanielLeens verified stays unchanged.

F1 (relative seatunnel_home): resolved once up front with os.path.abspath(os.path.expanduser(...)), so the script path and cwd can't be resolved twice. Code: install_plugins L286. Tests: test_relative_home_is_resolved_to_an_absolute_script_path (L460), test_invalid_seatunnel_home_raises_plugin_install_error (L348).
F2 (stray whitespace / trailing comments): mirrors the script's first-character rule and tolerates stray whitespace. A line the script would reject raises with its line number before any write. Code: select_plugins L91-153. Tests: test_connector_lines_with_stray_whitespace_are_recognized (L192), test_disabled_connector_with_trailing_whitespace_can_be_enabled (L206), test_line_rejected_by_install_script_raises_and_leaves_file_unchanged (L213).
F3 (inter-process lock): _exclusive_lock holds an exclusive flock across snapshot, select, run and restore. Code: _exclusive_lock L206, used at L300. Test: test_lock_is_held_while_the_script_runs (L501).
F4 (Windows): rejected before bash is looked up or any file is touched; the rationale is in the docstring (L233-235). Code: install_plugins L262. Test: test_windows_is_rejected_before_rewriting (L475).
F5 (version validation): validated against the script's character set before the rewrite. Code: install_plugins L273. Test: test_invalid_version_raises_before_rewriting (L449).
F6 (UnicodeDecodeError / TypeError leaks): wrapped into PluginInstallError. Code: select_plugins L120, _normalize_connectors L60. Tests: test_non_utf8_file_raises_plugin_install_error (L225), test_invalid_connectors_argument_raises_plugin_install_error (L234).
F7 (unconditional rollback write): _restore_if_changed writes only if the bytes differ. Code: L191-199, called at L327. Test: test_failed_install_does_not_rewrite_an_unchanged_file (L486).
F8 ('3.0.1' in the error hint): the hint now says only that the version should match the SeaTunnel engine version. Code: install_plugins L268-277. Test: test_empty_version_raises (L437).

All checks are green on 1c42f78 (Build, Python 3.9, Python 3.13).

SEZ9 · 2026-10-31T02:08:34Z

Thanks @karthik-0306 for the detailed mapping — it's very helpful for the re-review. Your comment appears to be cut off at the F8 entry (it ends mid-word at test_empty_version_rais); could you re-post the rest so I have the complete F8 reference?

On the docstring: yes, please add the select_plugins() clause to the install_plugins docstring so the cross-platform alternative is discoverable from the API docs and not only from the error message. Feel free to push it whenever you're ready.

I'll go through the F1–F8 changes against the diff and confirm each one here once I've had a chance to verify them.

DanielLeens · 2026-10-31T02:47:04Z

Thanks @karthik-0306 and @SEZ9. The full F8 entry is readable through the API: test_empty_version_raises at line 437; no repost is needed for my review. I also rechecked the current install_plugins docstring: it explains Linux/macOS support and the Windows rollback limitation, while the cross-platform select_plugins alternative is in the error message. Adding that alternative to the docstring is a useful documentation refinement as requested by @SEZ9. My prior source clearance is unchanged, and the reported Build/Python checks are green. A future documentation push will be assessed on its new head.

Checked 2026-10-31T02:46:59.551142+00:00; unchanged head 1c42f78cbda1242d35ee6e546d993b4017b30a36. Signal pull request update=SUCCESS; Test with Python 3.9=SUCCESS; Test with Python 3.13=SUCCESS; Build=SUCCESS.

This is a discussion/CI clarification, not another full source review or an effective maintainer approval. Daniel has READ repository permission; a write-capable maintainer owns required approval/dismissal.

…string

karthik-0306 · 2026-10-31T09:09:39Z

@SEZ9. Pushed: the install_plugins docstring now says that select_plugins() works on any platform, matching the Windows error message. It is a docstring-only change and the tests are unchanged. Since the line numbers below install_plugins move by one, the F1-F8 mapping above still applies with +1 for L262 and later. Ready for your final pass.

DanielLeens

Thank you again, @karthik-0306. I re-read the whole two-file diff at the new head 5e82f43, not just the increment. My previous approval on 1c42f78 stands: none of the earlier blockers (rollback on failure, interrupt handling, script-compatible parsing, locking, Windows rejection, version validation) has regressed, and the only change since then is a docstring edit. Because you asked for extra care on the dependency and download side, I traced that path against the real bin/install-plugin.sh on dev.

What Problem Does This PR Solve?

User pain point: the planned pip launcher (#12707) needs a programmatic, safe way to pick connectors in config/plugin_config and fetch them. Hand-editing the file or a failed download can leave the selection half-changed.
Fix approach: select_plugins() rewrites the file atomically (unrequested connectors get a leading #), and install_plugins() validates inputs, takes an exclusive lock, runs the existing installer script, and restores the original bytes on failure or interrupt.
One-sentence summary: a small, stdlib-only wrapper around the existing installer, with rollback and locking, that does not add any download logic of its own.

1. Code Change Review

1.1 Core Logic Analysis

Delta since 1c42f78: one commit that only rewords the install_plugins docstring so it also says that select_plugins() works on any platform (plugin_installer.py, docstring above install_plugins). No behavior change.

Runtime path (nothing in-tree calls it yet; the future launcher is the caller):

install_plugins(home, connectors, version)
  -> reject Windows; version must fullmatch [A-Za-z0-9._-]+         (plugin_installer.py:262-278)
  -> abspath(expanduser(home)); require config/plugin_config + bin/install-plugin.sh  (:280-295)
  -> shutil.which("bash")                                            (:297)
  -> _exclusive_lock(config/.plugin_config.lock)                     (:301)
       -> snapshot original bytes                                    (:303-308)
       -> select_plugins(): names normalized, lines classified like the script, atomic write only if changed  (:312)
       -> subprocess.run([bash, <abs script>, version], cwd=home)    (:314-316)
       -> on any BaseException: _restore_if_changed(), then re-raise (:325-338)

Dependency and download security review (this is the part I checked most carefully):

No new third-party dependency: the module imports only the standard library (contextlib, os, re, shutil, subprocess, sys, tempfile, typing; fcntl lazily on POSIX). Nothing is added to setup.py or requirements.txt, so there is no supply-chain surface change.
The Python code performs no network access and downloads nothing itself. All artifact fetching stays in bin/install-plugin.sh, which on dev enforces an HTTPS-only Maven repository, requires curl and a checksum tool, and verifies checksums of downloaded jars. The helper therefore inherits the script's integrity guarantees and cannot weaken them.
Command injection is closed: subprocess.run receives an argument list (no shell=True), version is restricted to [A-Za-z0-9._-]+ before use, and connector names are restricted to [A-Za-z0-9._][A-Za-z0-9._-]* and must additionally match a line already present in plugin_config. A name such as ../x or a;b is rejected before anything is written, so a caller-supplied name cannot become a path or a shell fragment.
SEATUNNEL_PLUGIN_DOWNLOAD_METHOD and SEATUNNEL_MAVEN_REPOSITORY are passed through from the caller's environment unchanged. That is the documented script behavior, and the script itself rejects a non-HTTPS repository, so this is acceptable. Worth keeping in mind for the launcher: it should not forward untrusted environment values.
File handling: temp file in the same directory, mode copied from the original, os.replace, temp file removed on failure. The lock file is intentionally left in place (documented reason). No world-writable files are created.

1.2 Compatibility Impact

Fully compatible. Two new files under tools/seatunnel-python-sdk; no engine, connector, shell script, config format, or public SDK export is changed.

1.3 Performance / Side-Effect Analysis

One file read, at most one atomic write on select and one on failure, one lock file descriptor. The flock can block while another process holds the copy, which is the intended serialization. No hot-path or memory concern.

1.4 Error Handling and Logging

Error contract is consistent (PluginInstallError for invalid input, undecodable file, missing layout, script failure, lock/write failure). Interrupts are re-raised as interrupts with the restore failure attached. The previous review points (mine and @SEZ9's F1-F8) are all addressed in the code as mapped in your comment; I verified each against the source rather than the description.

Issue 1 (non-blocking): the lock only protects callers that go through `install_plugins`

Location: tools/seatunnel-python-sdk/seatunnel/plugin_installer.py:23-26, 301
Problem: select_plugins() takes no lock and the lock is advisory, so a direct caller or someone running the shell script by hand can still interleave. The module docstring already states select_plugins takes no lock, so this is documented.
Risk: Low, and only for callers outside the intended cache-directory ownership model.
Best improvement: none needed now; keep the ownership-boundary wording when the launcher PR documents the API.
Severity: Low
Raised by another reviewer: No

2. Code Quality

2.1 Coding Standards / Comments

License headers, docstrings and explanatory comments are good, especially the BaseException and lock-file comments. The new docstring wording now matches the Windows error message, which was SEZ9's last request.

2.2 Tests and Stability

tests/test_plugin_installer.py was not changed in this increment. It uses temporary directories and mocks for subprocess.run / shutil.which; I found no sleep, thread timing, network or fixed-port use. Stability rating: Stable.

2.3 Documentation

No user-facing docs are required, since the helper is not exported from seatunnel/__init__.py. As agreed on #12707, the API docs belong in the launcher PR.

3. Architectural Soundness

This is the precise fix at the right layer: Python orchestration owns the rewrite, the lock and the error boundary, and the existing script contract is untouched. The alternative (teaching the script an explicit artifact list) would be cleaner long term but has a much larger compatibility cost and is not needed here. The #-disable design keeps the full connector list reusable for later calls. Maintainability and extensibility are fine for the launcher follow-up.

4. Issue Summary

Number	Issue	Location	Severity
1	Advisory lock only covers `install_plugins` callers	`plugin_installer.py:23-26, 301`	Low

5. Merge Recommendation

CI on 5e82f43: the contributor's fork Build is still running, and the apache-side PR signal and SeaTunnel Python SDK runs show action_required (waiting for a maintainer to approve the workflow). The previous head 1c42f78 was fully green (Build, Python 3.9, Python 3.13) and this commit touches only a docstring, so I expect the same result, but the current-head result is not yet available.

Conclusion: Ready to merge

Blockers - must be fixed
- None in the source. Only the current-head CI result is outstanding.
Recommended - non-blocking
- Issue 1 above, at your discretion (documentation only).

Overall: low-risk, well-tested, stdlib-only, and the download/dependency path stays entirely inside the already-hardened installer script. Thanks for the patient iterations.

SEZ9 · 2026-11-01T02:18:45Z

Thanks for the docstring-only push, @karthik-0306 — understood that install_plugins' docstring now states select_plugins() works on any platform, that it matches the Windows error message, and that the tests are unchanged. The +1 line shift from L262 onward is noted, so the F1-F8 references still line up on my side.

Before I can do the final pass I need one thing from you: a short per-finding status for F1-F8, since this comment only tells me about the docstring and I don't want to assume the rest. Concretely:

F1 (relative seatunnel_home resolving the script path twice under cwd=seatunnel_home) — addressed, or still open?
F2 (select_plugins leaving non-fully-matching connector lines enabled) — addressed, or still open?
F3 (no inter-process lock around snapshot → select → run → restore) — addressed, deferred, or still open?
F4 (hard bash + bin/install-plugin.sh requirement vs. bin/install-plugin.cmd on Windows) — the docstring tweak clarifies the contract, but does the code now avoid picking up a WSL/Git bash.exe and failing clearly on Windows, or is it still a documented limitation?
F5 (version only checked for emptiness) — addressed, or still open?
F6 (UnicodeDecodeError / TypeError leaking instead of PluginInstallError) — addressed, or still open?
F7 (unconditional rollback write of original) — addressed, or still open?
F8 ('3.0.1' example version in the error hint) — addressed, or still open?

A one-liner per item ("fixed", "won't fix because …", or "open") is enough. If any are fixed, please point me to where so I can verify them directly rather than guess. Once I have that list I'll go through the current state of tools/seatunnel-python-sdk/seatunnel/plugin_installer.py and close out whatever is resolved.

karthik-0306 · 2026-11-01T06:15:55Z

Thanks @SEZ9. Per-finding status at the current head 5e82f43 (all against tools/seatunnel-python-sdk/seatunnel/plugin_installer.py). All eight are fixed; none is open or deferred.

F1 — fixed. seatunnel_home is resolved once to an absolute path (L287), and bash gets that absolute script path with cwd=seatunnel_home, so the path can't be resolved twice. Tests: test_relative_home_is_resolved_to_an_absolute_script_path (L460), test_invalid_seatunnel_home_raises_plugin_install_error (L348).
F2 — fixed. select_plugins (L91-170) classifies lines the way install-plugin.sh does. Blank lines, "-" headers and "#" comments are skipped. Any other line must be a valid artifact ID, otherwise it raises with the line number before anything is written (L148-153), so a connector line with a trailing comment is no longer left enabled. Tests: test_connector_lines_with_stray_whitespace_are_recognized (L192), test_disabled_connector_with_trailing_whitespace_can_be_enabled (L206), test_line_rejected_by_install_script_raises_and_leaves_file_unchanged (L213).
F3 — fixed. _exclusive_lock (L207-227) takes an flock on config/.plugin_config.lock. install_plugins holds it (L301) across snapshot, select, run and restore. select_plugins() alone takes no lock; that is documented in the module docstring. Test: test_lock_is_held_while_the_script_runs (L501).
F4 — fixed in code, not only documented. Windows is rejected at L263 with a clear error that points to select_plugins(). This happens before shutil.which("bash") at L297, so a WSL or Git bash.exe is never picked up. The docstring (L233-236) now says the same thing and explains why install-plugin.cmd can't be used: it does not report download failures through its exit code, which would defeat the rollback. Test: test_windows_is_rejected_before_rewriting (L475).
F5 — fixed. The version is checked for emptiness (L270) and against the script's allowed characters [A-Za-z0-9._-] (L274), before anything is rewritten. Tests: test_empty_version_raises (L437), test_invalid_version_raises_before_rewriting (L449).
F6 — fixed. UnicodeDecodeError and OSError when reading the file (L120), a non-iterable connectors argument (L67-72), a non-string connector name (L76-77), and a non-string version (L269) all raise PluginInstallError. Tests: test_non_utf8_file_raises_plugin_install_error (L225), test_invalid_connectors_argument_raises_plugin_install_error (L234).
F7 — fixed. The 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 (L486).
F8 — fixed. The empty-version and invalid-version errors no longer name a version; they say it should match your SeaTunnel engine version (L271-277). Test: test_empty_version_raises (L437) asserts "engine version" is in the message and "3.0.1" is not.

Related tests (rollback on failure, interrupt, restore failure): L294, L303, L317, L326, L333, L372, L382.

DanielLeens · 2026-11-01T11:43:00Z

Thanks @karthik-0306 for the complete F1-F8 mapping. I checked the implementation pointers: absolute home resolution is at plugin_installer.py:287; line classification rejects invalid active artifacts before writing; _exclusive_lock spans snapshot/select/run/restore (:207-227,301); Windows rejection (:263) precedes bash discovery (:297); version validation (:269-277), normalized input/read errors, conditional rollback (:191-199,328) and the engine-version hint are present. This confirms the statuses already recorded in my full review. The advisory-lock boundary for callers bypassing install_plugins remains documented and non-blocking.

The earlier statement that the fork Build was still running is now obsolete: the exact-head fork Build has completed successfully. The current PR check rollup is empty, so I am not equating that fork result with an accepted Apache required check or effective maintainer approval. A write-capable maintainer should reconcile the matching fork run/Apache workflow permission and required-check discovery. My existing approval and source clearance stand; no new implementation or empty trigger commit is requested.

Checked 2026-11-01T11:42:54.249739+00:00; unchanged head 5e82f43015c46c8abeaa72994c8b4bd11fd4df27, base dev; compare diverged, ahead 7 / behind 22; merge blocked / BLOCKED; review decision REVIEW_REQUIRED. Behind/diverged is recorded independently and does not itself request a sync. Daniel has READ repository permission.

SEZ9 · 2026-11-02T02:13:20Z

Thanks for the detailed per-finding breakdown at 5e82f43 — the F1–F3 descriptions line up with what the review asked for:

F1 — resolving seatunnel_home to an absolute path once (L287) and passing that absolute script path with cwd=seatunnel_home removes the double resolution. The two tests you list (L460, L348) cover the relative-home and invalid-home cases, which is what I wanted to see.
F2 — classifying lines the way install-plugin.sh does, and raising with the line number before anything is written (L148-153), closes the trailing-whitespace / trailing-comment gap. The three tests at L192, L206 and L213 cover the right scenarios.
F3 — an flock on config/.plugin_config.lock via _exclusive_lock (L207-227), held by install_plugins (L301) across the whole sequence, is the right shape.

One thing: your comment appears to have been cut off mid-sentence in F3 ("across snapshot, select, ru…"), so I can't see the rest of the F3 explanation or anything you wrote for F4–F8. Could you repost the missing part? Specifically I'd like the same per-finding format for:

F3 (remainder) — confirm the lock is released on both the success and failure paths, and name the test(s) covering it.
F4 — how Windows is handled now (use of bin/install-plugin.cmd, or an explicit unsupported-platform error) and what prevents a WSL/Git bash.exe from being handed a Windows path.
F5 — what version validation rejects before the script starts ([, ,, +, LATEST, -SNAPSHOT, etc.).
F6 — that non-UTF-8 plugin_config and connectors=None now surface as PluginInstallError.
F7 — that the rollback only rewrites the file when select_plugins actually changed it.
F8 — the updated example in the error message.

Once I can see those, I'll go through the current head and close out the remaining items.

DanielLeens · 2026-11-02T02:41:14Z

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 yields 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.

karthik-0306 · 2026-11-02T09:02:25Z

Thanks @SEZ9 and @DanielLeens. @DanielLeens's reply above has the per-finding answers for F3-F8; they match my notes.

On the one open question, F3 lock release: _exclusive_lock releases the lock in a finally (os.close(fd), L226-227), so it is dropped on success, on PluginInstallError and on KeyboardInterrupt. test_lock_is_held_while_the_script_runs covers the held-while-running side; there is no dedicated test for release after a failure, because the finally guarantees it structurally. I'm happy to add one if you want it, but I haven't pushed anything, to keep the head you both reviewed (5e82f43).

DanielLeens · 2026-11-02T10:28:40Z

Thanks @karthik-0306 for the offer, and for holding the head at 5e82f43.

My view is that no extra test is needed for the lock-release point. I re-read _exclusive_lock in tools/seatunnel-python-sdk/seatunnel/plugin_installer.py at this head: the yield sits inside a try whose finally calls os.close(fd), and closing the descriptor drops the flock. That covers a normal return, PluginInstallError and KeyboardInterrupt structurally, and test_lock_is_held_while_the_script_runs already covers the held-while-running side. A dedicated post-failure test would be nice-to-have at most, so I would not push anything for it.

Please keep the PR unchanged: a new commit would force a fresh review round and refresh the approval and CI binding for no behavioural gain. My earlier approval stands for 5e82f43, and I have no outstanding source asks. What remains is on the committer side: the Apache-side check rollup for this head is currently empty (the exact-head fork Build was reported successful earlier), so a write-capable maintainer needs to reconcile the required check and give the final approval. As before, this is a static read; I did not execute anything locally.

karthik-0306 mentioned this pull request Oct 18, 2026

[Feature][Client] Make SeaTunnel installable and runnable from PyPI (pip install) #12707

Open

2 of 3 tasks

DanielLeens reviewed Oct 18, 2026

View reviewed changes

DanielLeens approved these changes Oct 19, 2026

View reviewed changes

github-actions Bot added the reviewed label Oct 19, 2026

DanielLeens reviewed Oct 19, 2026

View reviewed changes

karthik-0306 force-pushed the feature/python-sdk-install-plugin branch from 5e77800 to 8e61514 Compare October 23, 2026 18:35

DanielLeens reviewed Oct 24, 2026

View reviewed changes

DanielLeens approved these changes Oct 26, 2026

View reviewed changes

SEZ9 reviewed Oct 26, 2026

View reviewed changes

DanielLeens reviewed Oct 26, 2026

View reviewed changes

karthik-0306 mentioned this pull request Oct 26, 2026

[Fix][CI] Pin the Python SDK workflow to ubuntu-24.04 so Python 3.9 can be installed #12994

Merged

4 tasks

DanielLeens approved these changes Oct 27, 2026

View reviewed changes

karthik-0306 added 6 commits October 28, 2026 23:52

[Feature][Client] Add install-plugin helper to Python SDK

c636be6

[Feature][Client] Restore plugin_config when install-plugin.sh fails

d14ba56

[Feature][Client] Document plugin_config ownership and keep interrupt…

274a46b

…s on failed restore

[Feature][Client] Note that install-plugin.sh requires LF line endings

d7af5e3

[Feature][Client] Address review: absolute home, script-compatible pa…

d74fd2f

…rsing, lock, version check

[Feature][Client] Roll back an interrupt right after the rewrite and …

1c42f78

…validate seatunnel_home

karthik-0306 force-pushed the feature/python-sdk-install-plugin branch from 5c6b35f to 1c42f78 Compare October 28, 2026 18:22

[Feature][Client] Mention select_plugins() in the install_plugins doc…

5e82f43

…string

DanielLeens approved these changes Oct 31, 2026

View reviewed changes

Conversation

karthik-0306 commented Oct 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose of this pull request

Does this PR introduce any user-facing change?

How was this patch tested?

Check list

Uh oh!

DanielLeens left a comment

Choose a reason for hiding this comment

What Problem Does This PR Solve?

1. Code Change Review

1.1 Core Logic Analysis

Issue 1: A failed install leaves plugin_config changed

1.2 Compatibility Impact

1.3 Performance / Side-Effect Analysis

1.4 Error Handling and Logging

2. Code Quality Assessment

2.1 Coding Standards

2.2 Test Coverage and Test Stability

2.3 Documentation Updates

3. Architectural Soundness

3.1 Elegance of the Solution

3.2 Maintainability

3.3 Extensibility

3.4 Historical-Version Compatibility

4. Issue Summary

5. Merge Recommendation

Conclusion: Ready to merge after fixes

Uh oh!

karthik-0306 commented Oct 18, 2026

Uh oh!

DanielLeens left a comment

Choose a reason for hiding this comment

What Problem Does This PR Solve?

1. Code Change Review

1.1 Core Logic Analysis

1.2 Compatibility Impact

1.3 Performance / Side-Effect Analysis

1.4 Error Handling and Logging

Issue 1 (non-blocking): a failed restore during Ctrl+C turns the interrupt into PluginInstallError

2. Code Quality Assessment

2.1 Coding Standards

2.2 Test Coverage and Test Stability

2.3 Documentation Updates

3. Architectural Soundness

4. Issue Summary

5. Merge Recommendation

Conclusion: Ready to merge

Uh oh!

karthik-0306 commented Oct 19, 2026

Uh oh!

DanielLeens left a comment

Choose a reason for hiding this comment

What Problem Does This PR Solve?

1. Code Change Review

1.1 Core Logic Analysis

1.2 Compatibility Impact

1.3 Performance / Side-Effect Analysis

1.4 Error Handling and Logging

2. Code Quality Assessment

2.1 Coding Standards

2.2 Test Coverage and Test Stability

2.3 Documentation Updates

3. Architectural Soundness

4. Issue Summary

5. Merge Recommendation

Conclusion: Ready to merge

Uh oh!

karthik-0306 commented Oct 20, 2026

Uh oh!

DanielLeens commented Oct 21, 2026

Uh oh!

DanielLeens left a comment

Choose a reason for hiding this comment

What Problem Does This PR Solve?

1. Code Change Review

1.1 Core Logic Analysis

1.2 Compatibility Impact

1.3 Performance / Side-Effect Analysis

karthik-0306 commented Oct 18, 2026 •

edited

Loading

Issue 1: A failed install leaves `plugin_config` changed

Issue 1 (non-blocking): a failed restore during Ctrl+C turns the interrupt into `PluginInstallError`