DanielLeens commented on PR #11413:
URL: https://github.com/apache/seatunnel/pull/11413#issuecomment-5178113268
Thanks for asking before pushing ahead — that is exactly the right instinct
on a change this size, and it is a fair question. Two things: a CI correction I
owe you, then my actual answer.
## First, a correction: CI is green now
My previous comment asked you to sync with the latest `dev` and rerun
`Build`, because the run at that time had a red lane. **That is no longer the
current fact.** On head `ba85c1ab`, the required `Build` check is now
**SUCCESS**, along with `labeler` and `Notify test workflow`. So please
disregard the sync-and-rerun request — there is nothing to chase, and
re-rolling a green multi-hour connector CI result would only risk a flaky lane
for no gain.
Combined with my previous review, that means: **no source-level blocker from
my side, and CI is green.** The only open question on this PR is the one you
just asked.
## On the merge strategy
The decision is a committer's to make, so treat this as one reviewer's
opinion rather than an answer — but I have read the whole diff and I think the
shape of it points fairly clearly one way.
Here is what the 75 files actually are:
```text
seatunnel-connectors-v2/connector-hugegraph/src/main/... 37 files
seatunnel-connectors-v2/connector-hugegraph/src/test/... 18 files
seatunnel-e2e/.../connector-hugegraph-e2e/... 11 files
docs/{en,zh}/connectors/{source,sink,changelog}/... 6 files
pom.xml, plugin-mapping.properties, module poms 3 files
```
So despite +14135/-1223, this is almost entirely contained in one connector
module plus its own E2E and docs. It does not touch the engine, the API, other
connectors, or any shared runtime path. That matters a lot: the review risk of
a large PR is mostly about *blast radius*, not line count, and this one's blast
radius is genuinely small. SeaTunnel does merge large single-connector PRs for
exactly that reason.
**That said, I would still split it into two**, and the reason is the
`-1223`.
The two halves have very different risk profiles:
- **The new HugeGraph source is purely additive.** Nothing existing can
regress from it. If it has a bug, only people who opt into the new source are
affected.
- **The sink refactor with multi-mapping changes code that is already
released.** That is where every regression risk in this PR lives, and it is the
part that deserves a reviewer's full attention on its own terms — existing job
configs, existing mapping semantics, backward compatibility of the option
surface.
Bundled together, the sink refactor is reviewed as a supporting detail of a
large feature PR. Split apart, it gets reviewed as what it is: a change to
shipped behavior. My suggested order:
1. **PR A — sink refactor + multi-mapping**, including whatever shared
client/mapping layer the source will later build on. Reviewable against a
single question: does every existing HugeGraph sink job behave identically, and
where it does not, is that documented?
2. **PR B — the new source connector**, on top of A. Additive, and much
faster to review because the shared layer is already settled.
Two honest caveats so this is not a one-sided recommendation. First, if the
source and the refactored sink genuinely co-evolved, PR A may land a shared
abstraction with only one consumer, which is slightly awkward to review in
isolation — that is a real cost and if it turns out to be severe, say so and
keeping it as one PR is defensible. Second, splitting costs you a rebase and a
second CI cycle on an already-green branch. If a committer tells you to keep it
as one, that is a perfectly reasonable call given the contained blast radius,
and I would not argue with it.
Since your current head is green and unblocked, I would suggest waiting for
@davidzollo or @imbajin to weigh in before doing the split work — no point
rebasing twice.
## One suggestion on the design document
You linked the architecture and test results to an external wiki. That is
genuinely useful material and I am glad it exists — but for an ASF project it
is better if the design rationale lives somewhere the project itself retains:
the PR description, a linked GitHub issue, or a doc under `docs/`. External
links rot, and reviewers a year from now trying to understand why the mapping
model works the way it does will not have access to it. Even a condensed
version of the architecture and the multi-mapping semantics pasted into the PR
description would help — and it would help the committers answering your
question above, since PR scope decisions are much easier when the design
rationale is in front of them.
I have not opened the external link myself; the review above is based
entirely on the code in this PR.
Thanks again for the quality of this contribution and for checking on
process first — that is a good habit and it is appreciated. I will re-review
the full head from scratch whichever way the split decision goes.
--
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]
