This is an automated email from the ASF dual-hosted git repository. tkalkirill pushed a commit to branch ignite-26998 in repository https://gitbox.apache.org/repos/asf/ignite-3.git
commit cfe271baa46adf7ef23b21b4b0762dc6f8dbf576 Author: Kirill Tkalenko <[email protected]> AuthorDate: Thu Nov 27 15:03:39 2025 +0300 IGNITE-26998 wip --- .../table/distributed/gc/GcUpdateHandler.java | 10 ++----- .../ignite/internal/table/distributed/gc/MvGc.java | 4 +-- .../internal/table/distributed/IndexGcTest.java | 22 +++++++-------- .../gc/AbstractGcUpdateHandlerTest.java | 32 ++++++++++------------ .../internal/table/distributed/gc/MvGcTest.java | 12 ++++---- 5 files changed, 34 insertions(+), 46 deletions(-) diff --git a/modules/table/src/main/java/org/apache/ignite/internal/table/distributed/gc/GcUpdateHandler.java b/modules/table/src/main/java/org/apache/ignite/internal/table/distributed/gc/GcUpdateHandler.java index 4fb0358565e..5b239514e23 100644 --- a/modules/table/src/main/java/org/apache/ignite/internal/table/distributed/gc/GcUpdateHandler.java +++ b/modules/table/src/main/java/org/apache/ignite/internal/table/distributed/gc/GcUpdateHandler.java @@ -68,11 +68,9 @@ public class GcUpdateHandler { * * @param lowWatermark Low watermark for the vacuum. * @param count Count of entries to GC. - * @param strict {@code true} if needed to remove the strictly passed {@code count} oldest stale entries, {@code false} if a premature - * exit is allowed when it is not possible to acquire a lock for the {@link RowId}. * @return {@code False} if there is no garbage left in the storage. */ - public boolean vacuumBatch(HybridTimestamp lowWatermark, int count, boolean strict) { + public boolean vacuumBatch(HybridTimestamp lowWatermark, int count) { if (count <= 0) { return true; } @@ -88,11 +86,7 @@ public class GcUpdateHandler { case SUCCESS: return true; case FAILED_ACQUIRE_LOCK: - if (strict) { - continue; - } - - return true; + continue; case SHOULD_RELEASE: // Storage engine needs resources (e.g., checkpoint needs write lock). // Exit the closure to allow the engine to proceed. diff --git a/modules/table/src/main/java/org/apache/ignite/internal/table/distributed/gc/MvGc.java b/modules/table/src/main/java/org/apache/ignite/internal/table/distributed/gc/MvGc.java index c8bd08da27d..b3857f92e14 100644 --- a/modules/table/src/main/java/org/apache/ignite/internal/table/distributed/gc/MvGc.java +++ b/modules/table/src/main/java/org/apache/ignite/internal/table/distributed/gc/MvGc.java @@ -54,7 +54,7 @@ import org.jetbrains.annotations.TestOnly; /** * Garbage collector for multi-versioned storages and their indexes in the background. * - * @see GcUpdateHandler#vacuumBatch(HybridTimestamp, int, boolean) + * @see GcUpdateHandler#vacuumBatch */ public class MvGc implements ManuallyCloseable { private static final IgniteLogger LOG = Loggers.forClass(MvGc.class); @@ -252,7 +252,7 @@ public class MvGc implements ManuallyCloseable { }); currentAwaitSafeTimeFuture - .thenApplyAsync(unused -> gcUpdateHandler.vacuumBatch(lowWatermark, gcConfig.value().batchSize(), true), executor) + .thenApplyAsync(unused -> gcUpdateHandler.vacuumBatch(lowWatermark, gcConfig.value().batchSize()), executor) .whenComplete((isGarbageLeft, throwable) -> { if (throwable != null) { if (hasCause(throwable, TrackerClosedException.class, StorageRemovedException.class)) { diff --git a/modules/table/src/test/java/org/apache/ignite/internal/table/distributed/IndexGcTest.java b/modules/table/src/test/java/org/apache/ignite/internal/table/distributed/IndexGcTest.java index 669b69bea8e..498f4edefe1 100644 --- a/modules/table/src/test/java/org/apache/ignite/internal/table/distributed/IndexGcTest.java +++ b/modules/table/src/test/java/org/apache/ignite/internal/table/distributed/IndexGcTest.java @@ -42,7 +42,7 @@ public class IndexGcTest extends IndexBaseTest { addWrite(storageUpdateHandler, rowUuid, row); commitWrite(rowId); - assertTrue(gcUpdateHandler.vacuumBatch(now(), 1, true)); + assertTrue(gcUpdateHandler.vacuumBatch(now(), 1)); assertEquals(1, getRowVersions(rowId).size()); // Newer entry has the same index value, so it should not be removed. @@ -70,13 +70,13 @@ public class IndexGcTest extends IndexBaseTest { HybridTimestamp afterCommits = now(); - assertTrue(gcUpdateHandler.vacuumBatch(afterCommits, 1, true)); + assertTrue(gcUpdateHandler.vacuumBatch(afterCommits, 1)); // row1 should still be in the index, because second write was identical to the first. assertTrue(inAllIndexes(row1)); - assertTrue(gcUpdateHandler.vacuumBatch(afterCommits, 1, true)); - assertFalse(gcUpdateHandler.vacuumBatch(afterCommits, 1, true)); + assertTrue(gcUpdateHandler.vacuumBatch(afterCommits, 1)); + assertFalse(gcUpdateHandler.vacuumBatch(afterCommits, 1)); assertEquals(1, getRowVersions(rowId).size()); // Older entries have different indexes, should be removed. @@ -103,8 +103,8 @@ public class IndexGcTest extends IndexBaseTest { HybridTimestamp afterCommits = now(); - assertTrue(gcUpdateHandler.vacuumBatch(afterCommits, 1, true)); - assertFalse(gcUpdateHandler.vacuumBatch(afterCommits, 1, true)); + assertTrue(gcUpdateHandler.vacuumBatch(afterCommits, 1)); + assertFalse(gcUpdateHandler.vacuumBatch(afterCommits, 1)); assertEquals(0, getRowVersions(rowId).size()); // The last entry was a tombstone, so no indexes should be left. @@ -129,8 +129,8 @@ public class IndexGcTest extends IndexBaseTest { HybridTimestamp afterCommits = now(); - assertTrue(gcUpdateHandler.vacuumBatch(afterCommits, 1, true)); - assertFalse(gcUpdateHandler.vacuumBatch(afterCommits, 1, true)); + assertTrue(gcUpdateHandler.vacuumBatch(afterCommits, 1)); + assertFalse(gcUpdateHandler.vacuumBatch(afterCommits, 1)); assertEquals(1, getRowVersions(rowId).size()); assertTrue(inAllIndexes(row)); @@ -154,9 +154,9 @@ public class IndexGcTest extends IndexBaseTest { HybridTimestamp afterCommits = now(); - assertTrue(gcUpdateHandler.vacuumBatch(afterCommits, 1, true)); - assertTrue(gcUpdateHandler.vacuumBatch(afterCommits, 1, true)); - assertFalse(gcUpdateHandler.vacuumBatch(afterCommits, 1, true)); + assertTrue(gcUpdateHandler.vacuumBatch(afterCommits, 1)); + assertTrue(gcUpdateHandler.vacuumBatch(afterCommits, 1)); + assertFalse(gcUpdateHandler.vacuumBatch(afterCommits, 1)); assertEquals(0, getRowVersions(rowId).size()); assertTrue(notInAnyIndex(row)); diff --git a/modules/table/src/test/java/org/apache/ignite/internal/table/distributed/gc/AbstractGcUpdateHandlerTest.java b/modules/table/src/test/java/org/apache/ignite/internal/table/distributed/gc/AbstractGcUpdateHandlerTest.java index c7bb5da96c7..0f8fcd8db21 100644 --- a/modules/table/src/test/java/org/apache/ignite/internal/table/distributed/gc/AbstractGcUpdateHandlerTest.java +++ b/modules/table/src/test/java/org/apache/ignite/internal/table/distributed/gc/AbstractGcUpdateHandlerTest.java @@ -56,8 +56,6 @@ import org.apache.ignite.internal.util.PendingComparableValuesTracker; import org.jetbrains.annotations.Nullable; import org.junit.jupiter.api.RepeatedTest; import org.junit.jupiter.api.Test; -import org.junit.jupiter.params.ParameterizedTest; -import org.junit.jupiter.params.provider.ValueSource; import org.mockito.Mockito; /** @@ -82,9 +80,8 @@ abstract class AbstractGcUpdateHandlerTest extends BaseMvStoragesTest { this.tableStorage = tableStorage; } - @ParameterizedTest(name = "strict : {0}") - @ValueSource(booleans = {true, false}) - void testVacuum(boolean strict) { + @Test + void testVacuum() { TestPartitionDataStorage partitionStorage = spy(createPartitionDataStorage()); IndexUpdateHandler indexUpdateHandler = spy(createIndexUpdateHandler()); @@ -92,7 +89,7 @@ abstract class AbstractGcUpdateHandlerTest extends BaseMvStoragesTest { HybridTimestamp lowWatermark = HybridTimestamp.MAX_VALUE; - assertFalse(gcUpdateHandler.vacuumBatch(lowWatermark, 1, strict)); + assertFalse(gcUpdateHandler.vacuumBatch(lowWatermark, 1)); verify(partitionStorage).peek(lowWatermark); // Let's check that StorageUpdateHandler#vacuumBatch returns true. @@ -104,14 +101,13 @@ abstract class AbstractGcUpdateHandlerTest extends BaseMvStoragesTest { addWriteCommitted(partitionStorage, rowId, row, clock.now()); addWriteCommitted(partitionStorage, rowId, row, clock.now()); - assertTrue(gcUpdateHandler.vacuumBatch(lowWatermark, 1, strict)); + assertTrue(gcUpdateHandler.vacuumBatch(lowWatermark, 1)); verify(partitionStorage).peek(lowWatermark); verify(indexUpdateHandler).tryRemoveFromIndexes(any(), eq(rowId), any(), isNull()); } - @ParameterizedTest(name = "strict : {0}") - @ValueSource(booleans = {true, false}) - void testVacuumBatch(boolean strict) { + @Test + void testVacuumBatch() { TestPartitionDataStorage partitionStorage = spy(createPartitionDataStorage()); IndexUpdateHandler indexUpdateHandler = spy(createIndexUpdateHandler()); @@ -131,7 +127,7 @@ abstract class AbstractGcUpdateHandlerTest extends BaseMvStoragesTest { addWriteCommitted(partitionStorage, rowId1, row1, clock.now()); addWriteCommitted(partitionStorage, rowId1, row1, clock.now()); - assertFalse(gcUpdateHandler.vacuumBatch(lowWatermark, 5, strict)); + assertFalse(gcUpdateHandler.vacuumBatch(lowWatermark, 5)); verify(partitionStorage, times(3)).peek(lowWatermark); verify(indexUpdateHandler).tryRemoveFromIndexes(any(), eq(rowId0), any(), isNull()); @@ -162,8 +158,8 @@ abstract class AbstractGcUpdateHandlerTest extends BaseMvStoragesTest { addWriteCommitted(partitionStorage, rowId1, null, clock.now()); runRace( - () -> gcUpdateHandler.vacuumBatch(HybridTimestamp.MAX_VALUE, 2, true), - () -> gcUpdateHandler.vacuumBatch(HybridTimestamp.MAX_VALUE, 2, true) + () -> gcUpdateHandler.vacuumBatch(HybridTimestamp.MAX_VALUE, 2), + () -> gcUpdateHandler.vacuumBatch(HybridTimestamp.MAX_VALUE, 2) ); assertNull(partitionStorage.getStorage().closestRowId(RowId.lowestRowId(PARTITION_ID))); @@ -199,8 +195,8 @@ abstract class AbstractGcUpdateHandlerTest extends BaseMvStoragesTest { addWriteCommitted(partitionStorage, rowId3, null, clock.now()); runRace( - () -> gcUpdateHandler.vacuumBatch(HybridTimestamp.MAX_VALUE, 4, false), - () -> gcUpdateHandler.vacuumBatch(HybridTimestamp.MAX_VALUE, 4, false) + () -> gcUpdateHandler.vacuumBatch(HybridTimestamp.MAX_VALUE, 4), + () -> gcUpdateHandler.vacuumBatch(HybridTimestamp.MAX_VALUE, 4) ); assertNull(partitionStorage.getStorage().closestRowId(RowId.lowestRowId(PARTITION_ID))); @@ -243,7 +239,7 @@ abstract class AbstractGcUpdateHandlerTest extends BaseMvStoragesTest { } for (GcUpdateHandler gcUpdateHandler : gcUpdateHandlers) { - gcUpdateHandler.vacuumBatch(HybridTimestamp.MAX_VALUE, Integer.MAX_VALUE, true); + gcUpdateHandler.vacuumBatch(HybridTimestamp.MAX_VALUE, Integer.MAX_VALUE); } for (int i = 0; i < numPartitions; i++) { @@ -284,7 +280,7 @@ abstract class AbstractGcUpdateHandlerTest extends BaseMvStoragesTest { // When: Vacuum runs with shouldRelease() returning true (simulating lock contention) when(shouldReleaseSupplier.getAsBoolean()).thenReturn(true); - boolean hasGarbageLeft = gcUpdateHandler.vacuumBatch(lowWatermark, 10, true); + boolean hasGarbageLeft = gcUpdateHandler.vacuumBatch(lowWatermark, 10); // Then: Vacuum exits early and reports garbage remaining assertTrue(hasGarbageLeft, "Expected garbage to remain after early exit"); @@ -292,7 +288,7 @@ abstract class AbstractGcUpdateHandlerTest extends BaseMvStoragesTest { // When: Vacuum runs again with shouldRelease() returning false when(shouldReleaseSupplier.getAsBoolean()).thenReturn(false); - hasGarbageLeft = gcUpdateHandler.vacuumBatch(lowWatermark, 100, true); + hasGarbageLeft = gcUpdateHandler.vacuumBatch(lowWatermark, 100); // Then: All remaining garbage is processed assertFalse(hasGarbageLeft, "Expected no garbage left after completing vacuum"); diff --git a/modules/table/src/test/java/org/apache/ignite/internal/table/distributed/gc/MvGcTest.java b/modules/table/src/test/java/org/apache/ignite/internal/table/distributed/gc/MvGcTest.java index ce4c665081d..3ed0edd984c 100644 --- a/modules/table/src/test/java/org/apache/ignite/internal/table/distributed/gc/MvGcTest.java +++ b/modules/table/src/test/java/org/apache/ignite/internal/table/distributed/gc/MvGcTest.java @@ -29,9 +29,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; -import static org.mockito.ArgumentMatchers.anyBoolean; import static org.mockito.ArgumentMatchers.anyInt; -import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.spy; @@ -389,7 +387,7 @@ public class MvGcTest extends BaseIgniteAbstractTest { assertThat(startAwaitSafeTimeFuture, willCompleteSuccessfully()); assertThat(gc.removeStorage(tablePartitionId), willCompleteSuccessfully()); - verify(gcUpdateHandler, never()).vacuumBatch(any(), anyInt(), anyBoolean()); + verify(gcUpdateHandler, never()).vacuumBatch(any(), anyInt()); } private TablePartitionId createTablePartitionId() { @@ -409,7 +407,7 @@ public class MvGcTest extends BaseIgniteAbstractTest { CompletableFuture<Void> future, @Nullable HybridTimestamp exp ) { - when(gcUpdateHandler.vacuumBatch(any(HybridTimestamp.class), anyInt(), eq(true))).then(invocation -> { + when(gcUpdateHandler.vacuumBatch(any(HybridTimestamp.class), anyInt())).then(invocation -> { if (exp != null) { try { assertEquals(exp, invocation.getArgument(0)); @@ -429,7 +427,7 @@ public class MvGcTest extends BaseIgniteAbstractTest { private static GcUpdateHandler createWithCountDownOnVacuum(CountDownLatch latch) { GcUpdateHandler gcUpdateHandler = createGcUpdateHandler(); - when(gcUpdateHandler.vacuumBatch(any(HybridTimestamp.class), anyInt(), eq(true))).then(invocation -> { + when(gcUpdateHandler.vacuumBatch(any(HybridTimestamp.class), anyInt())).then(invocation -> { latch.countDown(); return latch.getCount() > 0; @@ -441,7 +439,7 @@ public class MvGcTest extends BaseIgniteAbstractTest { private static GcUpdateHandler createWithWaitFinishVacuum(CompletableFuture<Void> startFuture, CompletableFuture<Void> finishFuture) { GcUpdateHandler gcUpdateHandler = createGcUpdateHandler(); - when(gcUpdateHandler.vacuumBatch(any(HybridTimestamp.class), anyInt(), eq(true))).then(invocation -> { + when(gcUpdateHandler.vacuumBatch(any(HybridTimestamp.class), anyInt())).then(invocation -> { startFuture.complete(null); finishFuture.get(1, TimeUnit.SECONDS); @@ -461,7 +459,7 @@ public class MvGcTest extends BaseIgniteAbstractTest { private static GcUpdateHandler createWithCountDownOnVacuumWithoutNextBatch(CountDownLatch latch) { GcUpdateHandler gcUpdateHandler = createGcUpdateHandler(); - when(gcUpdateHandler.vacuumBatch(any(HybridTimestamp.class), anyInt(), eq(true))).then(invocation -> { + when(gcUpdateHandler.vacuumBatch(any(HybridTimestamp.class), anyInt())).then(invocation -> { latch.countDown(); // So that there is no processing of the next batch.
