[Feature] [Connectors-v2] Add Doris sink redirect enhancement#10715
[Feature] [Connectors-v2] Add Doris sink redirect enhancement#10715corgy-w merged 7 commits intoapache:devfrom
Conversation
DanielLeens
left a comment
There was a problem hiding this comment.
I pulled this PR locally and traced both the stream-load path and the 2PC control path.
The direct-to-BE wiring itself looks reasonable, but the new 307-diagnostic branches are very hard to reach in the real runtime because HttpUtil still enables DefaultRedirectStrategy for PUT and marks all methods redirectable (seatunnel-connectors-v2/connector-doris/src/main/java/org/apache/seatunnel/connectors/doris/util/HttpUtil.java:28). In practice the client will usually auto-follow the 307 before DorisStreamLoad.handlePreCommitResponse() or DorisCommitter.commitTransaction()/abortTransaction() can inspect the raw redirect response (.../DorisStreamLoad.java:221, .../DorisCommitter.java:83). If the redirected host is unreachable, the caller will still just see an IOException, not the new Location/stage diagnostics.
The current tests mock a raw 307 directly, so they prove the formatter works, but they do not prove the production client path can actually surface that information. Please either disable automatic redirect for these requests and handle 307 explicitly in the connector, or move the diagnostic enrichment into the redirect strategy / IO-error path so the promised redirect context is observable in production.
If the data synchronization volume is small, the 307 redirect issue will not be triggered. In fact, the HttpUtil method follows redirects by default. However, the 307 error occurs occasionally during large-volume data synchronization. Therefore, detailed exception logs have been added, and the relevant parameters for direct_to_be and benodes have also been supplemented. |
|
@yzeng1618 I think the blocker still stands. The key issue is not whether 307 happens often or only under heavy load. It is that the current production client path usually does not let the connector observe the raw 307 at all:
So in the real runtime, Apache HttpClient will typically follow the redirect before those branches execute. If the redirected target is bad or unreachable, the caller still just gets an That means the current tests prove the formatter logic, but they still do not prove the production path can surface the new diagnosis. I still think we need one of these before merge:
Without that, the new redirect troubleshooting branch remains mostly unreachable in the real path. |
I checked this again against the actual client path and updated the PR. The connector still keeps the existing auto-redirect behavior, but the Doris request execution path now uses I also added regression tests for the real production path ( So the diagnostics are now observable in the runtime path that previously only returned an |
DanielLeens
left a comment
There was a problem hiding this comment.
I re-checked the latest HEAD locally.
The blocker from my previous review looks addressed now:
- the production request path now goes through
HttpClientContext, so when Apache HttpClient auto-follows the FE -> BE redirect and the follow-up target fails at the IO layer, the connector can still surface Doris-specific redirect context (HttpUtil.java:53-79) DorisCommitter/DorisStreamLoadare wired to that tracked execution path (DorisCommitter.java:101-118,165-177;DorisStreamLoad.java:295-305)- the regression tests now cover the real runtime shape (
307 -> auto-follow -> follow-up connection failure) instead of only mocking a raw 307 (DorisCommitterTest.java:95-118and the corresponding stream-load test)
I do not see the previous observability gap in the current revision.
Nice follow-up on closing the real client-path issue.
|
Under the condition of direct_to_be=true && sink.enable-2pc=true, the writer first successfully creates the FE controlStreamLoad and then proceeds to create the BE dorisStreamLoad. If the second step fails, the constructor directly throws an exception, but the previously created controlStreamLoad will not be closed. Similarly, if create() succeeds and subsequent abortPreCommit() fails inside initializeStreamLoad(), the current DorisStreamLoad will also be leaked. |
For the resource leak, initializeLoad() now stages handles into local variables and wraps the entire init block in try/catch (RuntimeException). On failure, cleanupInitializedStreamLoads() closes whatever was already created, with an identity check to avoid double-closing. Both scenarios are covered by new unit tests. |
DanielLeens
left a comment
There was a problem hiding this comment.
I pulled the latest HEAD locally again. The redirect observability fix is still wired through the real Apache HttpClient path via HttpClientContext, and the follow-up initializeLoad() cleanup now stages both stream-load handles and closes the initialized ones on failure, so I do not see the previous Doris-side blockers in the current revision.
The current red Build is coming from seatunnel-engine-client (SeaTunnelEngineClusterRoleTest.testWorkerIsFirstMemberThenGetJobDetailStatus timeout in the unit-test job), which is outside the Doris connector files touched here.
DanielLeens
left a comment
There was a problem hiding this comment.
I re-pulled the latest HEAD locally and compared it against the previously reviewed Doris redirect revision.
On the current branch, the additional commits after my earlier approval are merge-from-dev only; the Doris connector files and Doris docs are unchanged relative to the revision I already checked.
So the earlier conclusion still stands:
- redirect diagnostics are still surfaced through the real HttpClient follow-up failure path
- staged stream-load handles are still cleaned up on init failure
- the current
Buildcheck is now green
I do not see a new blocker in the latest revision.
|
Hi @yzeng1618, I rechecked the current head locally as The runtime path remains: My previous approval still stands. The latest Conclusion: can merge |
Purpose of this pull request
This PR enhances Doris sink redirect handling and adds an opt-in direct-to-BE write path for
connector-doris.#10697
Does this PR introduce any user-facing change?
Yes.
Before this change, Doris sink always sent stream load requests to FE and relied on FE redirect. When users encountered a raw
307 Temporary Redirect, the connector only exposed limited diagnostics, and there was no explicit way to opt in to direct BE writes.After this change:
benodesanddirect_to_be=trueto send stream load write requests directly to Doris BEdirect_to_be=trueandsink.enable-2pc=true, data writes go to BE while commit/abort still go through FEdirect_to_be=truebutbenodesis missing or blankdirect_to_be/sink.enable-2pcstate, and troubleshooting guidanceHow was this patch tested?
Unit tests
E2E
Check list
New License Guide
incompatible-changes.mdto describe the incompatibility caused by this PR.