[PR] Fix IoTV2 receiver race and region migration retries [iotdb]

["Pengzna (via GitHub)" <gi...@apache.org>]

Pengzna opened a new pull request, #17512:
URL: https://github.com/apache/iotdb/pull/17512

   ## Summary
   - bound DELETE_OLD_REGION_PEER RPC retries and avoid adding duplicate destination peers when creating IoT/IoTV2 peers during region migration
   - make IoTConsensus createLocalPeer tolerate an existing consensus directory after cluster restart recovery
   - reserve IoTConsensusV2 receiver writers atomically before waiting so different commitIds cannot race onto the same writer, and wake waiters when a writer is returned
   
   ## Verification
   - mvn -pl integration-test -am -P with-integration-tests -DskipTests -Dtest.port.closed=true -Drat.skip=true -ntp compile
   - mvn verify -pl integration-test -am -P with-integration-tests,DailyIT -DskipUTs -Dtest.port.closed=true -DfailIfNoTests=false -Dfailsafe.failIfNoSpecifiedTests=false -Drat.skip=true -Dit.test=IoTDBIoTConsensusV2Batch3C3DBasicIT#testDeleteTimeSeriesReplicaConsistency,IoTDBIoTConsensusV2Stream3C3DBasicIT#testDeleteTimeSeriesReplicaConsistency,IoTDBRegionMigrateOriginalCrashWhenDeleteLocalPeerForIoTV2BatchIT,IoTDBRegionMigrateClusterCrashIoTV1IT#clusterCrash1 -ntp


-- 
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: reviews-unsubscribe@iotdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Fix region migration retries and peer recreation [iotdb]

["Pengzna (via GitHub)" <gi...@apache.org>]

Pengzna closed pull request #17512: Fix region migration retries and peer recreation
URL: https://github.com/apache/iotdb/pull/17512


-- 
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: reviews-unsubscribe@iotdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Fix IoTV2 receiver race and region migration retries [iotdb]

["Copilot (via GitHub)" <gi...@apache.org>]

Copilot commented on code in PR #17512:
URL: https://github.com/apache/iotdb/pull/17512#discussion_r3101844809


##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/pipe/receiver/protocol/iotconsensusv2/IoTConsensusV2Receiver.java:
##########
@@ -1017,13 +1017,25 @@ public IoTConsensusV2TsFileWriter borrowCorrespondingWriter(TCommitId commitId)
         // We should synchronously find the idle writer to avoid concurrency issues.
         lock.lock();
         try {
-          // We need to check tsFileWriter.isPresent() here. Since there may be both retry-sent
-          // tsfile
-          // events and real-time-sent tsfile events, causing the receiver's tsFileWriter load to
-          // exceed IOTDB_CONFIG.getIoTConsensusV2PipelineSize().
-          while (!tsFileWriter.isPresent()) {
+          while (true) {
+            tsFileWriter =
+                iotConsensusV2TsFileWriterPool.stream()
+                    .filter(
+                        item ->
+                            item.isUsed()
+                                && Objects.equals(
+                                    commitId, item.getCommitIdOfCorrespondingHolderEvent()))
+                    .findFirst();
+            if (tsFileWriter.isPresent()) {
+              break;
+            }

Review Comment:
   In `borrowCorrespondingWriter`, the fast-path (when an existing writer for the same commitId is already present) is still decided outside `IoTConsensusV2TsFileWriterPool.lock` and returned immediately. Since `releaseTsFileWriter`/`returnSelf` can concurrently flip `isUsed` and clear `commitIdOfCorrespondingHolderEvent` (without holding the pool lock), this method can return a writer that has just been returned to the pool and may be re-assigned to another commitId, reintroducing the race this PR is trying to eliminate. Consider performing the “find matching used writer / find idle writer / reserve” sequence under the same pool lock for all paths (including the existing-writer path), and re-validating the writer state before returning it (or introducing a ref-count / per-writer guard to prevent concurrent return/reuse).



##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/pipe/receiver/protocol/iotconsensusv2/IoTConsensusV2Receiver.java:
##########
@@ -1325,6 +1337,12 @@ private void releaseTsFileWriter(
     tsFileWriter.setWritingFile(null);
     try {
       tsFileWriter.returnSelf(consensusPipeName);
+      iotConsensusV2TsFileWriterPool.lock.lock();
+      try {
+        iotConsensusV2TsFileWriterPool.condition.signalAll();
+      } finally {
+        iotConsensusV2TsFileWriterPool.lock.unlock();
+      }

Review Comment:
   `releaseTsFileWriter` now signals waiters on the pool condition, but the state change that makes a writer available (`returnSelf` clearing commitId and setting `isUsed=false`) happens outside the pool lock. This leaves a window where borrowers holding the pool lock can observe intermediate states (e.g., `isUsed==true` but `commitId==null`) and unnecessarily go into `await`, defeating the purpose of signaling and making the correctness depend on timing/volatiles. To make the condition-based wait robust, consider guarding the writer “return to pool” state transition with the same pool lock used by `borrowCorrespondingWriter`, and then signal while still holding that lock.



-- 
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: reviews-unsubscribe@iotdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org