[To dev/1.3] Fix SyncStatus batch memory reservation retry - #18557
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev/1.3 #18557 +/- ##
=============================================
- Coverage 44.84% 44.83% -0.02%
Complexity 198 198
=============================================
Files 3663 3663
Lines 250109 250110 +1
Branches 30859 30858 -1
=============================================
- Hits 112168 112128 -40
- Misses 137941 137982 +41 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
| } | ||
| // Memory may be freed by another SyncStatus, which cannot notify this monitor. | ||
| wait(Math.max(1, config.getReplication().getBasicRetryWaitTimeMs())); | ||
| } |
There was a problem hiding this comment.
This retry loop fixes a liveness gap after batch memory reservation fails: memory can be released by another SyncStatus instance without notifying this monitor, so an unbounded wait could leave the dispatcher stuck. The timed wait rechecks reservation using the existing basicRetryWaitTimeMs while the separate pending-batch wait still enforces maxPendingBatchesNum. The change preserves the synchronized lifecycle and passed the SyncStatusTest and two-replica concurrent-write integration test.
| long previousMaxQueueMemory = memoryManager.getMaxMemorySizeForQueueInByte(); | ||
| ExecutorService executor = Executors.newSingleThreadExecutor(); | ||
| CountDownLatch taskStarted = new CountDownLatch(1); | ||
| memoryManager.init(0, 0); |
There was a problem hiding this comment.
This regression test forces the first reservation to fail with zero memory, verifies the writer remains pending, then raises the limit and requires completion within a timeout. It deterministically covers the retry path and restores the singleton memory-manager limits, executor, and SyncStatus state in finally so it remains isolated from neighboring tests. The targeted consensus Maven test suite passed with zero failures.
| .setBasicRetryWaitTimeMs(10) | ||
| // Only one large batch fits, forcing competing dispatchers to retry | ||
| // reservation. | ||
| .setAllocateMemoryForConsensus(REQUEST_PAYLOAD_SIZE * 2L) |
There was a problem hiding this comment.
This integration scenario starts two real IoTConsensus replicas and launches eight concurrent 8 MiB writes (four per replica) while the consensus memory cap admits only one two-entry batch. That deliberate contention exercises dispatcher reservation retries; the test then waits for both minSyncIndex values and all 16 state-machine deliveries, proving large concurrent requests do not deadlock synchronization. Dynamic port selection and duplicate writable buffers keep the test isolated and Thrift-compatible; the test passed in the consensus Maven module.
1 participant
Summary\n- Retry SyncStatus batch memory reservation when memory is temporarily unavailable.\n- Add a regression test covering a failed first reservation and a later successful retry.\n- Adapt the test to the memory manager API available on dev/1.3.\n\n## Test\n- mvn -pl iotdb-core/consensus -am -Dtest=SyncStatusTest -Dsurefire.failIfNoSpecifiedTests=false test\n- mvn -pl iotdb-core/consensus -am -DskipTests compile