DanielLeens commented on PR #10849:
URL: https://github.com/apache/seatunnel/pull/10849#issuecomment-4550420326
Thanks for continuing this. I re-reviewed the latest head from scratch, and
on the Bigtable connector path itself I do not see the earlier registration /
sink-path blockers reopened. The remaining blocker I want to call out is
different: the PR scope is no longer isolated to the Bigtable delivery.
## What this PR is trying to solve
- User pain point: SeaTunnel does not yet have an official Google Cloud
Bigtable Source/Sink connector.
- Fix approach: add the connector module, plugin/dist registration, docs,
and unit tests so Bigtable becomes a first-class Connector-V2 integration.
- One-line summary: the connector path is much healthier now, but the
current PR mixes unrelated common/UI cleanup into the same review unit, so it
is not merge-ready.
## Runtime path I re-traced
```text
Source path
-> BigtableSourceFactory
-> BigtableSourceReader
-> BigtableClient scan/read
-> SeaTunnelRow emission
Sink path
-> BigtableSinkFactory
-> BigtableSinkWriter
-> buffered mutations
-> Bigtable client flush/commit
Packaging path
-> plugin-mapping.properties
-> seatunnel-dist/pom.xml
-> docs/en + docs/zh
```
## Findings
### Issue 1: the PR scope now includes unrelated common/UI cleanup that is
outside the Bigtable runtime path
- Location:
-
`seatunnel-common/src/test/java/org/apache/seatunnel/common/utils/ReflectionUtilsTest.java:23-48`
- `seatunnel-engine/seatunnel-engine-ui/src/views/managers/index.tsx:17-40`
-
`seatunnel-engine/seatunnel-engine-ui/src/views/overview/baseInfo.tsx:18-25`
-
`seatunnel-engine/seatunnel-engine-ui/src/components/directed-acyclic-graph/index.tsx:337-343`
- Severity: High
- Why this is a problem:
- The Bigtable connector path does not execute through these files, but
they are now part of the same PR as the connector delivery.
- So the PR is no longer solving one reviewable problem. It is shipping a
connector plus unrelated cleanup in common/UI code.
- Risk:
- unrelated regressions can piggyback on a connector PR;
- rollback / revert / backport becomes coarser and riskier;
- the real Bigtable signal gets buried under unrelated noise.
- Best fix:
- keep the Bigtable connector delivery in this PR;
- split the `ReflectionUtilsTest` rewrite and the `seatunnel-engine-ui`
cleanup into separate PRs.
- Relation to earlier discussion:
- I agree with @nzw921rx's earlier points on registration / connector-path
correctness, and on the current head those specific Bigtable-path concerns look
resolved. The blocker I am flagging here is different: scope hygiene on the
latest head.
### Issue 2: the current Build is still red
- Location:
- GitHub Actions `Build`
- fork run `26449224731`
- Severity: Medium
- Why this is a problem:
- In the referenced fork workflow, I could confirm failing jobs including:
- `all-connectors-it-5 (8, ubuntu-latest)`
- `rocketmq-connector-it (8, ubuntu-latest)`
- `paimon-connector-it (8, ubuntu-latest)`
- Some parallel legs were also cancelled, so the CI signal is not yet in a
clean green state.
- Risk:
- the current head is still not mergeable from a CI gate perspective.
- Best fix:
- first clean up Issue 1 and rerun Build on the narrowed PR;
- then re-check whether any remaining failures are actually tied to the
Bigtable code path.
## Code quality / tests / docs
- The Bigtable connector code itself is reasonably structured.
- Test stability rating for the test changes in this PR: **Stable**.
- The Bigtable tests are mock/local-JVM oriented and I did not find
`Thread.sleep`, fixed-port contention, or shared-global-state patterns that
would make them flaky by design.
- Docs are present in both `docs/en` and `docs/zh`, which is the right
direction for a new user-visible connector.
## Conclusion
### Conclusion: can merge after fixes
1. Blockers
- Issue 1: please split the unrelated common/UI cleanup out of this PR so
the Bigtable connector can be reviewed and reverted on its own.
2. Suggested follow-ups
- Issue 2: rerun and inspect Build after the PR scope is cleaned up.
Overall, the Bigtable connector path is in much better shape now, but the
current review unit is still too broad for Apache-style merge hygiene.
--
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]
