Re: [PR] fix: retry SyncStatus batch memory reservation [iotdb]
jt2594838 merged PR #18453: URL: https://github.com/apache/iotdb/pull/18453 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fix: retry SyncStatus batch memory reservation [iotdb]
sonarqubecloud[bot] commented on PR #18453: URL: https://github.com/apache/iotdb/pull/18453#issuecomment-5264521256 ## [](https://sonarcloud.io/dashboard?id=apache_iotdb&pullRequest=18453) **Quality Gate failed** Failed conditions  [B Security Rating on New Code](https://sonarcloud.io/dashboard?id=apache_iotdb&pullRequest=18453) (required ≥ A)  [E Reliability Rating on New Code](https://sonarcloud.io/dashboard?id=apache_iotdb&pullRequest=18453) (required ≥ A) [See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_iotdb&pullRequest=18453) ##  Catch issues before they fail your Quality Gate with our IDE extension  [SonarQube for IDE](https://www.sonarsource.com/products/sonarlint/features/connected-mode/?referrer=pull-request) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fix: retry SyncStatus batch memory reservation [iotdb]
jt2594838 commented on code in PR #18453:
URL: https://github.com/apache/iotdb/pull/18453#discussion_r3764914940
##
iotdb-core/consensus/src/test/java/org/apache/iotdb/consensus/iot/logdispatcher/SyncStatusTest.java:
##
@@ -242,4 +251,59 @@ public void waitTest() throws InterruptedException,
ExecutionException {
Assert.assertEquals(
config.getReplication().getMaxPendingBatchesNum() + 1,
status.getNextSendingIndex());
}
+
+ @Test
+ public void testFirstBatchRetriesMemoryReservation()
+ throws InterruptedException, ExecutionException, TimeoutException {
+IndexController controller =
+new IndexController(storageDir.getAbsolutePath(), peer, 0,
CHECK_POINT_GAP);
+IoTConsensusConfig retryConfig =
+IoTConsensusConfig.newBuilder()
+.setReplication(
+
IoTConsensusConfig.Replication.newBuilder().setBasicRetryWaitTimeMs(10).build())
+.build();
+SyncStatus status = new SyncStatus(controller, retryConfig);
+TLogEntry logEntry = new TLogEntry().setSearchIndex(1).setMemorySize(1);
+Batch batch = new Batch(retryConfig);
+batch.addTLogEntry(logEntry);
+batch.buildIndex();
+
+IoTConsensusMemoryManager memoryManager =
IoTConsensusMemoryManager.getInstance();
+IMemoryBlock previousMemoryBlock = memoryManager.getMemoryBlock();
+CountDownLatch firstAllocationFailed = new CountDownLatch(1);
+AtomicBoolean rejectAllocation = new AtomicBoolean(true);
+IMemoryBlock memoryBlock =
+new AtomicLongMemoryBlock("SyncStatusTest", null,
batch.getMemorySize()) {
+ @Override
+ public boolean allocate(long sizeInByte) {
+if (rejectAllocation.compareAndSet(true, false)) {
+ firstAllocationFailed.countDown();
+ return false;
+}
+return super.allocate(sizeInByte);
+ }
+};
+ExecutorService executor = Executors.newSingleThreadExecutor();
+memoryManager.setMemoryBlock(memoryBlock);
+try {
+ Future future =
+ executor.submit(
+ () -> {
+status.addNextBatch(batch);
+return null;
+ });
+
+ Assert.assertTrue(firstAllocationFailed.await(5, TimeUnit.SECONDS));
Review Comment:
This regression test deterministically rejects the first allocation, then
waits for `addNextBatch` to retry without any external notification. The
bounded latches/future make a permanent wait fail quickly, and the `finally`
block restores the singleton memory block so the test cannot leak global state
into other consensus tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] fix: retry SyncStatus batch memory reservation [iotdb]
jt2594838 commented on code in PR #18453:
URL: https://github.com/apache/iotdb/pull/18453#discussion_r3764893304
##
iotdb-core/consensus/src/main/java/org/apache/iotdb/consensus/iot/logdispatcher/SyncStatus.java:
##
@@ -49,10 +49,15 @@ public SyncStatus(IndexController controller,
IoTConsensusConfig config) {
* @throws InterruptedException
*/
public synchronized void addNextBatch(Batch batch) throws
InterruptedException {
-while ((pendingBatches.size() >=
config.getReplication().getMaxPendingBatchesNum()
-|| !iotConsensusMemoryManager.reserve(batch))
-&& !Thread.interrupted()) {
- wait();
+while (true) {
+ while (pendingBatches.size() >=
config.getReplication().getMaxPendingBatchesNum()) {
+wait();
+ }
+ if (iotConsensusMemoryManager.reserve(batch)) {
+break;
+ }
+ // Memory may be freed by another SyncStatus, which cannot notify this
monitor.
+ wait(Math.max(1, config.getReplication().getBasicRetryWaitTimeMs()));
Review Comment:
A failed reservation cannot rely on this monitor being notified: memory may
be released by another `SyncStatus`, and the first batch has no local
completion callback. Retrying after the configured basic interval guarantees
progress while the separate queue-capacity loop still uses `removeBatch()`
notifications. Letting `wait` propagate `InterruptedException` also prevents an
interrupted dispatcher from enqueueing a batch whose memory was never reserved.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
[PR] fix: retry SyncStatus batch memory reservation [iotdb]
jt2594838 opened a new pull request, #18453: URL: https://github.com/apache/iotdb/pull/18453 ## Description ### Fix SyncStatus memory reservation retry When the first batch fails to reserve consensus memory, its `SyncStatus` has no pending batch whose completion can call `notifyAll()`. The dispatcher can therefore wait indefinitely even after memory becomes available elsewhere. This change separates pending-batch backpressure from memory reservation. A full pending queue still waits for `removeBatch()` notification, while a failed memory reservation waits for at least 1 ms using the configured basic retry interval and then retries. It also preserves normal `InterruptedException` semantics instead of clearing the interrupt and potentially enqueueing an unreserved batch. ### Regression coverage Adds a unit test with a memory block that rejects the first allocation and accepts the second, verifying that the first batch progresses without an external notification and that reserved memory is released afterward. Validation: - `mvn test -pl iotdb-core/consensus -Dtest=SyncStatusTest` - `mvn test -pl iotdb-core/consensus -Dtest=SyncStatusTest -P with-zh-locale` - Spotless and Checkstyle passed in both runs This PR has: - [x] been self-reviewed. - [x] concurrent write - [x] added comments explaining the why and the intent of the code wherever it would not be obvious for an unfamiliar reader. - [x] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage. # Key changed/added classes - `SyncStatus`: retry failed batch memory reservations without relying on another batch to notify the monitor. - `SyncStatusTest`: cover first-batch allocation failure and recovery. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
