SEZ9 commented on PR #12537: URL: https://github.com/apache/seatunnel/pull/12537#issuecomment-5931240271
Thanks for the review — this was a genuinely useful one. I reproduced all three issues before fixing them, and all three were real. Pushed as 9eb6061. ### Issue 1 (High) — mixed-case namespaces: fixed Your list was exactly right. I enumerated every error-code literal in the tree to confirm the scope: ``` grep -rhoE '"[A-Za-z][A-Za-z0-9_]*-[0-9]+"' --include='*ErrorCode.java' . ``` 69 distinct namespace prefixes, **13 of which are not SCREAMING_CASE**: ``` AmazonSqs AzureQueueStorage Bigquery Bigtable Doirs Doris GooglePubSub GraphQL Hbase RedisErrorCode S3RedShift SelectDB TDengine ``` (`Doirs` is a real typo'd variant that ships today, which is a good argument on its own for not relying on a naming convention here.) Confirmed before the fix that `ErrorCode:[Doris-06], ErrorDescription:[backend client error]` produced `code=None, component=None, category="unknown"` — a total miss on well-formed input. Broadened the namespace group to `[A-Za-z][A-Za-z0-9_]*` and verified all 13 now parse. The `ErrorCode:[` literal prefix keeps this from widening what else can match. ### Issue 2 (Medium) — signature collapse: fixed Confirmed `JDBC-02` and `JDBC-05` both produced `"JDBC-?"`. Took your suggested fix — when a code is present it is used as-is, and `_VOLATILE_RE` now only applies to the exception/raw-text fallbacks. Verified `JDBC-01`..`JDBC-20` now yield 20 distinct signatures. ### Issue 3 (Medium) — non-str input: fixed Confirmed `int`/`list`/`dict` raised `AttributeError` and `bytes` raised `TypeError`. I went with your preferred option — the guard lives inside `parse_error` rather than relying on call sites — for exactly the reason you gave: this function's whole job is to run while the CLI is already reporting a failure, so it must never be the thing that hides the user's real error. ### On the truncation ordering — agreed, and thank you for catching it This is the most valuable point in the review and I want to confirm it explicitly rather than just nod at it. `cli.py:1411` is `error_text[:3000]`, which keeps the **front** of the trace, while Java prints the outer wrapper first and the innermost `Caused by` last — precisely the field this module exists to rescue. Worth noting the same file already does tail-truncation elsewhere (`proc.stdout[-3000:]` at `cli.py:1529`), so the front-truncation here looks more like an oversight than a deliberate choice. The wiring PR will call `parse_error` on the untruncated `error_text` and will include a test with a >3000-char trace whose root cause sits at the end. I'm leaving that in the follow-up rather than pulling it in here to keep this PR to the single file under review. ### One small correction The review says both that nothing imports `seatunnel_cli.diagnostics` yet (correct — I re-confirmed) and that `cli.py:1411` is "the only real caller". `cli.py:1411` is the *prospective* call site; there is no caller today. Doesn't change any of the conclusions. Full suite is green: 205 passed (202 before, +3 regression tests). Each new test was confirmed to fail against the pre-fix code so none of them are vacuous. The new test data uses codes and descriptions copied verbatim from `DorisConnectorErrorCode`, `GraphQLConnectorErrorCode` and `JdbcConnectorErrorCode` rather than invented strings. -- 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]
