Re: [PR] PipeConsensus: Support deletion operation for PipeConsensus [iotdb]

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

Caideyipi commented on code in PR #13016:
URL: https://github.com/apache/iotdb/pull/13016#discussion_r1792962546


##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/pipe/extractor/dataregion/historical/PipeHistoricalDataRegionTsFileAndDeletionExtractor.java:
##########
@@ -573,6 +611,27 @@ private boolean isTsFileGeneratedAfterExtractionTimeLowerBound(final TsFileResou
     }
   }
 
+  private void extractDeletions(
+      final DeletionResourceManager deletionResourceManager,
+      final List<PersistentResource> resourceList) {
+    LOGGER.info("Pipe {}@{}: start to extract deletions", pipeName, dataRegionId);
+    long startTime = System.currentTimeMillis();
+    List<DeletionResource> allDeletionResources = deletionResourceManager.getAllDeletionResources();
+    final int originalDeletionCount = allDeletionResources.size();
+    allDeletionResources =
+        allDeletionResources.stream()
+            .filter(resource -> !startIndex.isAfter(resource.getProgressIndex()))
+            .collect(Collectors.toList());
+    resourceList.addAll(allDeletionResources);

Review Comment:
   Seemingly the resources' reference counts are not considered here? The deletion files may not be actually deleted at all after restart....



##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/pipe/extractor/dataregion/historical/PipeHistoricalDataRegionTsFileAndDeletionExtractor.java:
##########
@@ -251,7 +258,8 @@ public void customize(
       final PipeParameters parameters, final PipeExtractorRuntimeConfiguration configuration)
       throws IllegalPathException {
     shouldExtractInsertion = listeningOptionPair.getLeft();
-    // Do nothing if only extract deletion
+    shouldExtractDeletion = listeningOptionPair.getRight();
+    // Do nothing if extract deletion

Review Comment:
   If only extract deletion? Now for the user pipes only-extracting deletion, the persisted deletion can be used too. However this may cause extra deletion if the sender pipes restart too late and the receiver has some data locally written to it...



##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/pipe/extractor/dataregion/historical/PipeHistoricalDataRegionTsFileAndDeletionExtractor.java:
##########
@@ -573,6 +611,27 @@ private boolean isTsFileGeneratedAfterExtractionTimeLowerBound(final TsFileResou
     }
   }
 
+  private void extractDeletions(
+      final DeletionResourceManager deletionResourceManager,
+      final List<PersistentResource> resourceList) {
+    LOGGER.info("Pipe {}@{}: start to extract deletions", pipeName, dataRegionId);
+    long startTime = System.currentTimeMillis();
+    List<DeletionResource> allDeletionResources = deletionResourceManager.getAllDeletionResources();
+    final int originalDeletionCount = allDeletionResources.size();
+    allDeletionResources =
+        allDeletionResources.stream()
+            .filter(resource -> !startIndex.isAfter(resource.getProgressIndex()))
+            .collect(Collectors.toList());
+    resourceList.addAll(allDeletionResources);

Review Comment:
   BTW, all the followers, whether received or not, will receive the deletion after restart. It seems OK, but I'm not sure that if the improvement deserves to be done.



-- 
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