ibessonov commented on code in PR #1952:
URL: https://github.com/apache/ignite-3/pull/1952#discussion_r1173834642
##########
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:
How is it possible to have bad safe time inside of the GC task? Shouldn't we
only add task to the pool if low watermark is good? Otherwise we end up adding
a task that only adds another task, what's the point of it?
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/StorageUpdateHandler.java:
##########
@@ -41,44 +40,51 @@
import org.apache.ignite.internal.table.distributed.raft.PartitionDataStorage;
import
org.apache.ignite.internal.table.distributed.replicator.TablePartitionId;
import org.apache.ignite.internal.util.Cursor;
+import org.apache.ignite.internal.util.PendingComparableValuesTracker;
import org.jetbrains.annotations.Nullable;
/**
* Handler for storage updates that can be performed on processing of primary
replica requests and partition replication requests.
*/
public class StorageUpdateHandler {
- /** Partition id. */
Review Comment:
Was it necessary to remove this comment? Code, where some fields have
comments, and others don't, looks ugly in my opinion. Another opinion - don't
change the code that's absolutely fine already
--
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]
