tkalkirill commented on code in PR #1952:
URL: https://github.com/apache/ignite-3/pull/1952#discussion_r1182111891


##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/gc/MvGc.java:
##########
@@ -221,34 +222,62 @@ private void scheduleGcForStorage(TablePartitionId 
tablePartitionId) {
                 return;
             }
 
-            if (storageHandler.gcInProgressFuture.get() != future) {
+            if (storageHandler.gcInProgressFuture.get() != currentGcFuture) {
                 // Someone in parallel is already collecting garbage, we will 
try once again after completion of gcInProgressFuture.
                 return;
             }
 
             try {
-                for (int i = 0; i < GC_BATCH_SIZE; i++) {
-                    HybridTimestamp lowWatermark = lowWatermarkReference.get();
+                HybridTimestamp lowWatermark = lowWatermarkReference.get();
 
-                    assert lowWatermark != null : tablePartitionId;
+                assert lowWatermark != null : tablePartitionId;
 
-                    // If storage has been deleted or there is no garbage, 
then for now we will stop collecting garbage for this storage.
-                    if 
(!storageHandlerByPartitionId.containsKey(tablePartitionId)
-                            || 
!storageHandler.storageUpdateHandler.vacuum(lowWatermark)) {
-                        return;
-                    }
-                }
-            } catch (Throwable t) {
-                future.completeExceptionally(t);
+                // If the storage has been deleted, then garbage collection is 
no longer necessary.
+                if 
(!storageHandlerByPartitionId.containsKey(tablePartitionId)) {
+                    currentGcFuture.complete(null);
 
-                return;
-            } finally {
-                if (!future.isCompletedExceptionally()) {
-                    future.complete(null);
+                    return;
                 }
-            }
 
-            scheduleGcForStorage(tablePartitionId);
+                StorageUpdateHandler storageUpdateHandler = 
storageHandler.storageUpdateHandler;
+
+                // We can only start garbage collection when the partition 
safe time is reached.
+                storageUpdateHandler.getSafeTimeTracker()
+                        .waitFor(lowWatermark)

Review Comment:
   I don't quite understand what bad safe time means.
   Since the safe time is a distributed property of each partition, we have to 
wait for it, don't we?
   
   The future safe time could complete in a raft thread, so it's safer to move 
the garbage collection task to the garbage collector's pool.



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

Reply via email to