SEZ9 commented on PR #12858: URL: https://github.com/apache/seatunnel/pull/12858#issuecomment-6432245668
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. <!-- streview-comment:2065 --> -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
