SEZ9 commented on PR #12251: URL: https://github.com/apache/seatunnel/pull/12251#issuecomment-5946888332
Thanks @goutamadwant for resolving the `dev` docs conflict in `251a76af9d9a` without changing the JsonPath behaviour, and for keeping the CI notes scoped rather than claiming a green aggregate build. Before I can close out the earlier review points, I'd like to confirm the current state directly against the diff rather than from summaries. Could you help with the following? - **ROUTE_TO_TABLE on the conversion-error path (F3)** and **docs coverage of `row_error_handle_way` / "Configure error data handle way" (F4)**: please point me to where in the current head these sections document the conversion case and the ROUTE_TO_TABLE behaviour, and which test pins it (I understand this landed around `74defe3d5`/`cba69e7c`, but I want to verify at `251a76af9d9a`). - **Default `FAIL` compatibility note (F5)**: please confirm the note is present in both `incompatible-changes.md` files at the current head. - **Cause attachment (F8)**: I understand `d121f9e7685` is docs-only. Can you state how the cause is attached in the current code (constructor vs. `initCause()`), and if it is `initCause()`, why that is preferred over the existing `SeaTunnelRuntimeException` cause constructors? Not blocking, just want the decision on record. - **Test duplication / reflection into a private field (F6)**: still open as Low. If you want to keep the separate class, please restate the reason here so it isn't lost; otherwise folding the cases into `JsonPathTransformTest#testErrorHandleWay` would be my preference. Two items I have no update on: 1. **Full source JSON in the exception message (F1/F7)**: is the entire record (plus the converter's own copy) still embedded in the message that propagates to engine logs / job failure status? If yes, please consider truncating or omitting the payload, or explain why keeping it is intended. 2. **Blanket `catch (RuntimeException e)` (F2)**: can you confirm that `isDataConversionFailure` narrows the skippable set so only recognised data-conversion failures are nulled at DEBUG and everything else still propagates? Once CI for `251a76af9d9a` finishes, please ping here with the result and I'll do the final pass. <!-- streview-comment:1472 --> -- 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]
