[fix](logstash) Add HTTP timeouts and SO_KEEPALIVE to prevent pipeline hang#65766
[fix](logstash) Add HTTP timeouts and SO_KEEPALIVE to prevent pipeline hang#65766smallhibiscus wants to merge 1 commit into
Conversation
…e hang (1.2.1 -> 1.2.2) ### What problem does this PR solve? Issue Number: close #xxx Related PR: apache#63181 Problem Summary: logstash-output-doris 1.2.0 hung forever on TCP half-open connections because Future.get() and HttpAsyncClient had no timeouts. apache#63181 migrated to HttpClient4 sync with NoConnectionReuseStrategy, which removes the Future.get() hang and avoids keep-alive connection reuse, but RequestConfig still lacked connect/socket timeouts and SO_KEEPALIVE was not enabled. Without socketTimeout, @client.execute() can still block forever when the peer dies mid-request. This patch (1.2.1 -> 1.2.2): - set connectTimeout / connectionRequestTimeout / socketTimeout on RequestConfig - enable SO_KEEPALIVE via SocketConfig - expose connect_timeout_ms, connection_request_timeout_ms, socket_timeout_ms as configurable options (defaults: 60s / 60s / 600s) ### Release note logstash-output-doris 1.2.2: add HTTP connect/socket timeouts and SO_KEEPALIVE to prevent worker threads hanging forever on TCP half-open connections. New optional configs: connect_timeout_ms, connection_request_timeout_ms, socket_timeout_ms. ### Check List (For Author) - Test: - Manual test / No need to test (config-only HttpClient setup; timeout behavior matches Apache HttpClient4 RequestConfig / SocketConfig) - Behavior changed: Yes (HTTP requests now fail with timeout instead of hanging) - Does this need documentation: No (README updated in this PR) Co-authored-by: Cursor <[email protected]>
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
There was a problem hiding this comment.
Pull request overview
This PR updates the extension/logstash Logstash output plugin (logstash-output-doris) to prevent indefinite hangs on TCP half-open connections by configuring Apache HttpClient4 request timeouts and enabling SO_KEEPALIVE, and bumps the gem version accordingly.
Changes:
- Add configurable HttpClient4
connect,connection request, andsocket/readtimeouts (with defaults) and enableSO_KEEPALIVE. - Log the configured timeout values at plugin initialization for easier troubleshooting.
- Bump plugin version to
1.2.2and document the new configuration options in the README.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| extension/logstash/README.md | Documents the new timeout-related configuration options and defaults (v1.2.2+). |
| extension/logstash/logstash-output-doris.gemspec | Bumps gem version from 1.2.1 to 1.2.2. |
| extension/logstash/lib/logstash/outputs/doris.rb | Adds new plugin configs and applies them via RequestConfig and SocketConfig in the HttpClient4 builder. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
run buildall |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes: four blocking issues remain in the timeout implementation: shared-client pool starvation, invalid numeric coercion, no request-body write deadline, and a labeled-retry data-loss race.
Critical checkpoint conclusions:
- Goal and proof: The patch wires connect, pool-lease, and read timeouts plus
SO_KEEPALIVE, but does not yet provide bounded, safe execution because the four inline issues remain. No automated plugin test proves the new behavior. - Scope: The change is focused across the Ruby implementation, README, and gemspec; the documented defaults and version bump agree.
- Concurrency: Shared pipeline workers and the retry thread use one thread-safe client, but its unsized per-route pool creates
MAIN-01. No additional data race, deadlock, or lock-order issue was found. - Lifecycle and static state: No new static initialization or cycle is introduced. Existing retry/report/client shutdown paths predate this patch; an in-flight body write can still remain blocked as described by
MAIN-03. - Configuration: The settings are additive registration-time options with no persistence requirement, but their accepted value domain is unsafe as described by
MAIN-02. - Compatibility: No storage, wire-format, public-function-symbol, or rolling-upgrade boundary changes. Existing configurations receive the new defaults, and the added HttpClient APIs exist in the declared dependency.
- Parallel paths and conditions: Worker and retry callers, FE redirects, regular Stream Load, and Group Commit were checked. Group Commit's pre-existing at-least-once ambiguous retry is not reported as a new regression; regular labeled reconciliation is
MAIN-04. - Tests and results: No extension tests cover the new timeout domain, pool saturation, write stalls, or retry race. Ruby syntax checks passed, and independent JRuby 9.2/9.4 probes reproduced
MAIN-02. Per the review instructions, no build or cluster test was run. - Observability: Startup and request-failure logging exists, but fractional settings can log a value different from the effective Java timeout, covered by
MAIN-02. - Transactions and data writes: There is no EditLog or persisted-metadata change. Regular Stream Load can lose a batch after a nonterminal label collision, covered by
MAIN-04; Group Commit retains its documented at-least-once boundary. - FE/BE variables: None are added or changed.
- Performance: The default connection-pool/lease mismatch can create repeated retries under ordinary shared concurrency, covered by
MAIN-01. - Other correctness: The socket timeout does not bound synchronous request-body writes, covered by
MAIN-03. - User focus: No additional focus was supplied.
Review completion: the exact four inline comments accompany this summary. Three full rounds of normal and risk-focused review converged on this set, with every candidate either accepted, merged, or explicitly dismissed.
| config :max_retry_queue_mb, :validate => :number, :default => java.lang.Runtime.get_runtime.max_memory / 1024 / 1024 / 5 | ||
|
|
||
| # HTTP connect timeout in milliseconds (time to establish TCP connection) | ||
| config :connect_timeout_ms, :validate => :number, :default => 60_000 |
There was a problem hiding this comment.
These three public settings accept a wider domain than the Java API they feed. Logstash's :number accepts fractions, and on both README-supported JRuby lines 0.5 is truncated to Java 0; HttpClient interprets zero as an infinite timeout. Negative values likewise leave the finite-timeout guarantee undefined/disabled, while values above 2147483647 raise RangeError during register. The startup log prints the original Ruby value, so it can even claim 0.5 ms while the effective timeout is infinite. Please validate positive whole milliseconds in 1..2147483647, pass an explicit Java int, document the range, and add JRuby-facing boundary tests for all three options.
| # - SocketConfig SO_KEEPALIVE : let the kernel probe dead peers even without reuse | ||
| request_config = RequestConfig.custom | ||
| .setConnectTimeout(@connect_timeout_ms) | ||
| .setConnectionRequestTimeout(@connection_request_timeout_ms) |
There was a problem hiding this comment.
Please size the connection pool before making pool leases expire after 60 seconds. HttpClients.custom defaults to only 2 simultaneous connections per route (20 total), while this plugin is concurrency :shared and the retry thread uses the same client. With one effective FE/BE route and two valid loads running for more than 60 seconds, a third worker times out here with ConnectionPoolTimeoutException without sending its request; handle_request then requeues it, and request exceptions bypass max_retries. This turns normal shared concurrency into an unbounded retry cycle. Configure maxConnPerRoute/maxConnTotal for the supported worker count (or keep the lease wait consistent with valid in-flight loads) and cover three same-route executions in a test.
| request_config = RequestConfig.custom | ||
| .setConnectTimeout(@connect_timeout_ms) | ||
| .setConnectionRequestTimeout(@connection_request_timeout_ms) | ||
| .setSocketTimeout(@socket_timeout_ms) |
There was a problem hiding this comment.
setSocketTimeout only configures SO_TIMEOUT for InputStream.read; it does not time the synchronous ByteArrayEntity upload inside @client.execute. If the server returns 100 Continue and then stops consuming a batch larger than the TCP send buffers (for example, a live zero-window peer), the worker blocks in OutputStream.write; SO_KEEPALIVE also cannot abort that live connection. The advertised anti-hang behavior therefore still misses the request-body phase. Please enforce an overall/cancellable request deadline that closes/aborts the request while it is being written, or narrow the README/release claim to response-read hangs.
| request_config = RequestConfig.custom | ||
| .setConnectTimeout(@connect_timeout_ms) | ||
| .setConnectionRequestTimeout(@connection_request_timeout_ms) | ||
| .setSocketTimeout(@socket_timeout_ms) |
There was a problem hiding this comment.
A timeout-driven retry is not safe merely because it reuses the label. The retried HTTP request gets a new Stream Load request ID; if the original transaction is still PREPARE, Doris returns Label Already Exists with ExistingJobStatus: RUNNING. The existing handler treats every label collision as terminal success and discards this batch, but the original can still fail/abort afterward, leaving the rows permanently lost. Please retain and reconcile RUNNING/PRECOMMITTED collisions (poll until FINISHED, or retry the same label if it aborts) and only treat a finished existing job as success. Add a race test where the original is running at the collision and then aborts.
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #63181
Problem Summary:
logstash-output-doris 1.2.0 hung forever on TCP half-open connections because Future.get() and HttpAsyncClient had no timeouts. #63181 migrated to HttpClient4 sync with NoConnectionReuseStrategy, which removes the Future.get() hang and avoids keep-alive connection reuse, but RequestConfig still lacked connect/socket timeouts and SO_KEEPALIVE was not enabled. Without socketTimeout, @client.execute() can still block forever when the peer dies mid-request.
This patch (1.2.1 -> 1.2.2):
Release note
logstash-output-doris 1.2.2: add HTTP connect/socket timeouts and SO_KEEPALIVE to prevent worker threads hanging forever on TCP half-open connections. New optional configs: connect_timeout_ms, connection_request_timeout_ms, socket_timeout_ms.
Check List (For Author)
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)