dybyte commented on PR #11577: URL: https://github.com/apache/seatunnel/pull/11577#issuecomment-5264535814
> Thanks for the update. I re-reviewed the current head `cef445cd50c4746823f05f32d9a04d383d65d9aa` against the SeaTunnel schema-event execution path and BigQuery's production constraints. CI is green and the single-writer happy path is well covered, but I still see two merge-blocking issues. > > 1. **High — every sink subtask can execute the same DDL, which can exceed BigQuery's per-table metadata-update quota.** > > Location: `BigQuerySchemaChangeManager.java:82-103`, especially the check-then-query path at lines 87-96. > > SeaTunnel broadcasts a schema change to every sink subtask (for example, Flink's `BroadcastSchemaSinkOperator` forwards it to each sink writer). Each BigQuery writer owns its own manager, observes the column as missing, and calls `bigQuery.query(ALTER TABLE ...)`. `IF NOT EXISTS` makes the DDL logically idempotent, but it does not prevent the concurrent DDL job stampede. BigQuery limits table metadata updates, including table DDL, to 5 operations per 10 seconds per table. A sink parallelism above 5 can therefore fail a valid ADD COLUMN event with `Exceeded rate limits: too many table update operations for this table`; the current generic catch immediately fails the task and does not re-read/retry. > > Please coordinate the DDL so only one subtask applies each table/event, or add a bounded exponential-backoff path that re-reads the table and treats the event as successful once the expected compatible schema is visible. Add a parallel-writer regression test that exercises more than five concurrent handlers for the same event. > > 2. **High — existing DECIMAL and STRUCT columns are accepted as compatible without checking their actual schema.** > > Location: `BigQuerySchemaChangeManager.java:227-244`. > > `validateCompatibleField` compares only top-level `StandardSQLTypeName` and mode. That treats `NUMERIC(10,2)` as compatible with an expected `DECIMAL(20,2)`, and treats any `STRUCT` as compatible with any other `STRUCT`. The manager then skips DDL as idempotently complete. Later rows can fail only when a value exceeds the narrower precision/scale, or immediately when nested field names/types/modes differ. > > Please compare `Field.getPrecision()` / `getScale()` for NUMERIC/BIGNUMERIC and recursively compare `getSubFields()` for ROW / ARRAY, including nested names, types, and modes. Add tests for an existing narrower decimal and an incompatible nested struct. > > The overall design—flush old-schema rows, apply DDL, rebuild serializer/writer, then tolerate Storage Write API propagation—is reasonable and the English/Chinese docs are aligned. However, the two cases above affect correctness under supported production configurations, so I do not think this PR is ready to merge yet. @yzeng1618 The current schema change path does not broadcast DDL to every sink writer. However, I agree that it is beneficial to protect against possible duplicate DDL execution, so I added retry and schema re-check logic. This is covered by unit tests without adding a separate E2E test. -- 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]
