Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a new Apache Doris Stream Load output (doris_stream_load) and wires it into the community component bundle so it’s available in standard builds.
Changes:
- Adds a new
doris_stream_loadbatch output implementation with FE→BE redirect handling, JSON/CSV encoding, and FE failover support. - Registers the Doris component in
public/componentsand the plugin catalog. - Adds unit tests covering encoding, redirects/failover, header mapping, and connection tests.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| public/components/doris/package.go | Adds the public component package that side-effect imports the internal Doris implementation. |
| public/components/community/package.go | Wires the new Doris public component into the community build import set. |
| internal/plugins/info.csv | Registers doris_stream_load in the plugin inventory. |
| internal/impl/doris/output_stream_load.go | Implements the Doris Stream Load output, config parsing/spec, request building, redirect/failover logic, and connection tests. |
| internal/impl/doris/output_stream_load_test.go | Adds unit tests for encoding, redirects, failover, connection tests, and promoted header behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| BackoffMaxRetries: 3, | ||
| Retry: httpclient.DefaultRetryConfig(), | ||
| MetricPrefix: "doris_stream_load_http", | ||
| } |
There was a problem hiding this comment.
The HTTP client uses httpclient.DefaultTransportConfig(), which sets ExpectContinueTimeout=1s. Since requests always set Expect: 100-continue when a body is present, servers that don’t send a 100 response (common) will cause up to ~1s of extra latency before the body is transmitted. Consider explicitly setting cfg.Transport.ExpectContinueTimeout to 0 (send immediately) or a much smaller value tuned for Doris, while still keeping the Expect header required by Doris.
| } | |
| } | |
| cfg.Transport.ExpectContinueTimeout = 0 |
| resp, err := d.client.Do(req) | ||
| if err != nil { | ||
| lastErr = fmt.Errorf("connecting to Doris FE %s: %w", feURL, err) | ||
| continue | ||
| } | ||
| resp.Body.Close() | ||
| if d.conf.QueryPort > 0 { | ||
| if err := d.connectionCheck(ctx, feURL, d.conf.QueryPort); err != nil { | ||
| lastErr = err | ||
| continue | ||
| } | ||
| } | ||
| return service.ConnectionTestSucceeded().AsList() | ||
| } |
There was a problem hiding this comment.
ConnectionTest currently treats any HTTP response as success (it doesn’t check resp.StatusCode). This can incorrectly report success for cases like 401/403 (bad credentials) or 404, even though the output won’t actually work. Consider requiring a 2xx (or at least <400) status code and returning ConnectionTestFailed when the FE responds with an error status (optionally including the status/body in the error).
Summary
doris_stream_loadoutputExpect: 100-continue, query-port based connection tests, and FE failover viafe_urlsValidation
go test ./internal/impl/dorisgo build -o target/redpanda-connect ./cmd/redpanda-connectNotes