davidzollo commented on PR #10306: URL: https://github.com/apache/seatunnel/pull/10306#issuecomment-4841595925
I reviewed the latest PR head `d4bb2475a08cac4acc3c9c4e3c09213a77d4938e`. I do not think this is ready to merge yet. ## Blocking issue ### 1. Transform error-handler wrappers break schema-change propagation When `env.transform_error_handler` is enabled, `TransformFlowLifeCycle.initErrorHandlingTransforms()` replaces each `SeaTunnelMapTransform` / `SeaTunnelFlatMapTransform` with `ErrorHandlingMapTransform` or `ErrorHandlingFlatMapTransform`: - `seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/task/flow/TransformFlowLifeCycle.java:354-365` Later, the schema-change path refreshes each downstream transform from the previous transform's produced catalog tables before calling `mapSchemaChangeEvent(...)`: - `seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/task/flow/TransformFlowLifeCycle.java:141-153` However, the new wrappers only delegate methods such as `open`, `getProducedCatalogTables`, `mapSchemaChangeEvent`, `close`, `getPluginName`, and `setJobContext`. They do **not** delegate `setInputCatalogTables(...)`: - `seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/task/error/ErrorHandlingMapTransform.java:111-147` - `seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/task/error/ErrorHandlingFlatMapTransform.java:112-148` `SeaTunnelTransform.setInputCatalogTables(...)` has a default no-op implementation: - `seatunnel-api/src/main/java/org/apache/seatunnel/api/transform/SeaTunnelTransform.java:68` So after wrapping, the engine calls `setInputCatalogTables(...)` on the wrapper, the call is swallowed by the default no-op, and the real underlying transform does not receive the refreshed upstream schema. This breaks live schema-change chains when transform error handling is enabled. A concrete affected path is SQL transform. `SQLTransform` depends on input schema refresh to invalidate its cached SQL engine and output catalog: - `seatunnel-transforms-v2/src/main/java/org/apache/seatunnel/transform/sql/SQLTransform.java:219-251` Without forwarding `setInputCatalogTables(...)`, downstream SQL / Metadata / FilterField-style transform chains can keep stale schemas after `ALTER ADD COLUMN`, which can drop new columns or produce wrong row layouts. Existing schema-change tests cover the raw transform chain, but they do not appear to cover the same chain after it has been wrapped by `ErrorHandlingMapTransform` / `ErrorHandlingFlatMapTransform`. Suggested fix: make both wrappers delegate the full `SeaTunnelTransform` lifecycle/default-method surface to the underlying transform, at least `setInputCatalogTables(...)`; I would also delegate `setTypeInfo(...)` to avoid the same wrapper-default trap on older paths. Please add a regression test that enables the transform error handler, wraps a schema-change-capable transform chain, replays `ALTER ADD COLUMN`, and verifies the new columns still survive through the chain. ## Additional contract risk ### 2. `mode = ROUTE` silently downgrades to `LOG` when no error sink is configured `ErrorHandlerConfigUtil` force-downgrades `ROUTE` to `LOG` if `sink.plugin_name` is missing: - `seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/task/error/ErrorHandlerConfigUtil.java:105-115` After that, handled row-level errors are logged but not written to a DLQ/error sink. Since the bad row is still consumed by the error-handling path and removed from the normal output path, a user can configure `ROUTE`, get a successful job, and still have no dead-letter records. The docs describe this behavior, but I think it is a dangerous contract for a feature advertised as dead-letter/error-data routing. Suggested fix: fail fast during config validation when `mode = ROUTE` has no valid error sink, or require an explicit `mode = LOG` configuration for log-only behavior. I did not run a full Maven build or E2E suite for this review. The blocking issue above is a static lifecycle/delegation bug visible from the current source path. -- 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]
