DanielLeens commented on PR #11338:
URL: https://github.com/apache/seatunnel/pull/11338#issuecomment-4892009090
Rechecked the latest head `028df6b403` again. The stderr-collector cleanup
added in this revision is a good improvement, but the two earlier Daniel
blockers are still open on the current version.
## What changed in this new revision
- `PythonProcessWorker.close()` now waits for the stderr collector thread to
stop.
- `PythonTransformTest` adds coverage for the direct
`PythonTransform.close()` path.
That is directionally good, but it still does not close the two
production-path lifecycle gaps from the previous round:
1. **`PythonTransform` still keeps the old `PythonProcessWorker` across
schema changes.**
Files: `PythonTransform.java:38-39,108-113`,
`PythonProcessWorker.java:76-135`
The worker still captures `inputRowType`, `rowToJsonConverter`,
`outputConverters`, and the Python-side context when it is first created. I
still do not see a schema-change path that closes and rebuilds that worker
after the upstream `CatalogTable` changes, so the transform can keep
serializing rows with stale schema metadata on the next records.
2. **`PythonMultiCatalogTransform` still does not forward `close()` to the
inner `PythonTransform` instances.**
Files: `PythonTransformFactory.java:82-85`,
`PythonMultiCatalogTransform.java:29-75`
The production factory path still returns `PythonMultiCatalogTransform`,
and that wrapper still does not override `close()`. So even though the direct
`PythonTransform.close()` path is better now, the real factory-returned runtime
object still does not guarantee that the inner Python worker processes are
released.
The new test only covers the direct inner transform close path. It still
does not cover:
- the factory-returned wrapper close path
- the schema-change rebuild path
So from my side the merge conclusion is unchanged on the latest head:
- fix required before merge
- blocking items:
1. rebuild the cached Python worker after schema changes
2. ensure the outer multi-table wrapper actually closes all inner Python
transforms
Once those two blockers are fixed on top of this revision, I’m happy to
re-review again.
--
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]
