fix: race condition when attaching futures to a terminating stream#103
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request improves bulk write timeout diagnostics by logging request context (table, request ID, row count, timeout) and deduplicating timeout reporting between caller-side waits and internal deadline enforcement. It also bumps the project version to the next snapshot and tightens publishing workflow behavior for manual runs.
Changes:
- Add timeout-context logging and deduplication in the bulk write pipeline (
BulkWriteService,BulkWriteClient). - Add tests validating caller-timeout logging propagation and “log once” behavior.
- Bump Maven versions to
0.15.1-SNAPSHOT, update Central publishing plugin, and add guarded manual publishing workflow dispatch.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pom.xml | Bumps project version and updates Central publishing plugin version. |
| ingester-rpc/pom.xml | Aligns module parent version to 0.15.1-SNAPSHOT. |
| ingester-protocol/src/main/java/io/greptime/BulkWriteClient.java | Wraps bulk write futures to log caller-side timeouts with request context. |
| ingester-protocol/src/test/java/io/greptime/BulkWriteClientTest.java | Adds unit test ensuring timed get(...) reports timeout to the put stage. |
| ingester-protocol/pom.xml | Aligns module parent version to 0.15.1-SNAPSHOT. |
| ingester-prometheus-metrics/pom.xml | Aligns module parent version to 0.15.1-SNAPSHOT. |
| ingester-integration-tests/pom.xml | Aligns module parent version to 0.15.1-SNAPSHOT. |
| ingester-grpc/pom.xml | Aligns module parent version to 0.15.1-SNAPSHOT. |
| ingester-example/pom.xml | Aligns module parent version to 0.15.1-SNAPSHOT. |
| ingester-common/pom.xml | Aligns module parent version to 0.15.1-SNAPSHOT. |
| ingester-bulk-protocol/src/main/java/io/greptime/BulkWriteService.java | Adds table-name-aware timeout diagnostics and deduplicates deadline vs caller timeout logs. |
| ingester-bulk-protocol/src/main/java/io/greptime/BulkWriteManager.java | Passes table name into BulkWriteService for diagnostics. |
| ingester-bulk-protocol/src/test/java/io/greptime/BulkWriteServiceTest.java | Adds test ensuring request-context timeout log is emitted only once. |
| ingester-bulk-protocol/pom.xml | Aligns module parent version to 0.15.1-SNAPSHOT. |
| ingester-all/pom.xml | Aligns module parent version to 0.15.1-SNAPSHOT. |
| .github/workflows/mvn_publish.yml | Adds manual workflow dispatch and rejects manual publishes for non--SNAPSHOT versions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…r-failure permit leak Replace the one-shot weakly consistent terminal fanout with a lock-protected terminal state machine in `AsyncPutListener` so `attach()` and `onError()`/`onCompleted()` cannot race. When `putNext()` throws synchronously, the unsent Arrow metadata buffer is released and the write-limiter permit is returned immediately. - `ingester-bulk-protocol/src/main/java/io/greptime/BulkWriteService.java` - `ingester-protocol/src/main/java/io/greptime/limit/AbstractLimiter.java` - `ingester-bulk-protocol/src/test/java/io/greptime/BulkWriteServiceTest.java` - `ingester-protocol/src/test/java/io/greptime/WriteLimitTest.java` Signed-off-by: Lei, HUANG <ratuthomm@gmail.com>
fengjiachun
left a comment
There was a problem hiding this comment.
Reviewed the concurrency fix carefully — the core design holds up well: terminate() snapshots and clears futuresInFlight under the lock but completes futures outside it (no callback-under-lock deadlock risk); the re-entrancy path is safe since a failed future's callback hits the terminal check in onError() and returns false; and failing pending futures on normal completion instead of silently clearing them is the right call — that silent clear was part of the original leak. The AbstractLimiter permit-leak fix and the blockingWriteLimitTest rewrite (the old in-thread assertion could never actually fail the test) are both good catches. I ran all 18 new/changed tests locally and they pass.
Two issues that can't be anchored to the diff:
1. The new BulkWriteServiceTest never runs in CI. build.yml only has test jobs for ingester-common, ingester-grpc, ingester-protocol, and integration tests — there's no job for ingester-bulk-protocol, so the main regression suite of this PR won't run anywhere automated. Also, these tests fail on JDK 9+ without --add-opens=java.base/java.nio=ALL-UNNAMED (reproduced on Java 21: 4 errors from MemoryUtil initialization; all 11 pass with the flag). Suggest adding a test_ingester_bulk_protocol job, plus a JDK-9+-activated profile setting surefire argLine so local runs on modern JDKs work too.
2. The release workflow guard is one-directional. Manual dispatch requires a -SNAPSHOT version, but the release: created path has no inverse check — creating a GitHub release while main sits at 0.15.1-SNAPSHOT would publish a snapshot through the release pipeline. A symmetric "release requires non-SNAPSHOT" step would close that hole.
Remaining line-specific notes are inline. Overall LGTM once #1 is addressed — as written, the regression tests protecting this fix won't run in CI.
|
@fengjiachun Addressed the two unanchored review findings from this review:
Verified with the full Java 8 reactor test suite, |
Summary
BulkStreamWriterwrites time out.0.15.1-SNAPSHOTwith Central plugin 0.11.0 and a guarded manual workflow.AsyncPutListenerterminal-stream/attach race (closes AsyncPutListener can miss terminal stream errors for newly attached batches #105): introduce a lock-protected terminal state machine soattach()andonError()/onCompleted()cannot race. SynchronousputNext()failures now release the unsent Arrow metadata buffer and return the write-limiter permit immediately.Test Plan
mvn testmvn spotless:checkJAVA_HOME=... mvn -P release -DskipTests -Dgpg.skip=true clean verify