ibessonov commented on code in PR #1952:
URL: https://github.com/apache/ignite-3/pull/1952#discussion_r1173403012
##########
modules/table/src/test/java/org/apache/ignite/internal/table/distributed/AbstractMvStorageUpdateHandlerTest.java:
##########
@@ -95,7 +96,8 @@ void setUp(
PARTITION_ID,
partitionDataStorage,
DummyInternalTableImpl.createTableIndexStoragesSupplier(Map.of()),
- distributionZoneConfig.dataStorage()
+ distributionZoneConfig.dataStorage(),
+ new PendingComparableValuesTracker<>(HybridTimestamp.MAX_VALUE)
Review Comment:
MAX_VALUE?
Interesting
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/raft/PartitionListener.java:
##########
@@ -221,7 +228,8 @@ private void handleUpdateCommand(UpdateCommand cmd, long
commandIndex, long comm
txsPendingRowIds.computeIfAbsent(cmd.txId(), entry -> new
HashSet<>()).add(rowId);
storage.lastApplied(commandIndex, commandTerm);
- }
+ },
+ lowWatermarkSupplier.get()
Review Comment:
Why do we force "storageUpdateHandler" to perform GC, while we could do it
ourselves right here? Has such option been considered?
##########
modules/table/src/test/java/org/apache/ignite/internal/table/distributed/StorageUpdateHandlerTest.java:
##########
@@ -104,6 +118,56 @@ void testBuildIndex() {
verify(indexes, times(2)).addIndexToWaitIfAbsent(indexId);
}
+ @Test
+ void testVacuum() {
+ PartitionDataStorage partitionStorage = createPartitionDataStorage();
+
+ StorageUpdateHandler storageUpdateHandler =
createStorageUpdateHandler(partitionStorage,
mock(TableIndexStoragesSupplier.class));
+
+ HybridTimestamp lowWatermark = new HybridTimestamp(100, 100);
+
+ assertFalse(storageUpdateHandler.vacuum(lowWatermark));
+ verify(partitionStorage).pollForVacuum(lowWatermark);
+ // Let's check that StorageUpdateHandler#vacuumBatch returns true.
+ clearInvocations(partitionStorage);
+
+ BinaryRowAndRowId binaryRowAndRowId = new
BinaryRowAndRowId(mock(BinaryRow.class), new RowId(PARTITION_ID));
+
+
when(partitionStorage.scanVersions(any(RowId.class))).thenReturn(CursorUtils.emptyCursor());
+
when(partitionStorage.pollForVacuum(lowWatermark)).thenReturn(binaryRowAndRowId);
+
+ assertTrue(storageUpdateHandler.vacuum(lowWatermark));
+ verify(partitionStorage).pollForVacuum(lowWatermark);
+ }
+
+ @Test
+ void testExecuteBatchGc() {
+ PartitionDataStorage partitionStorage = createPartitionDataStorage();
+
+ StorageUpdateHandler storageUpdateHandler =
createStorageUpdateHandler(partitionStorage,
mock(TableIndexStoragesSupplier.class));
+
+ // Let's check that if lwm is {@code null} then nothing will happen.
+ storageUpdateHandler.executeBatchGc(null);
Review Comment:
Not obvious. What are the circumstances, in which we would pass `null` here?
Only tests?
##########
modules/table/src/test/java/org/apache/ignite/internal/table/distributed/StorageUpdateHandlerTest.java:
##########
@@ -129,4 +193,13 @@ private void setRowVersions(PartitionDataStorage
partitionStorage, Map<UUID, Lis
when(partitionStorage.scanVersions(rowId)).thenReturn(Cursor.fromIterable(readResults));
}
}
+
+ private static PartitionDataStorage createPartitionDataStorage() {
+ PartitionDataStorage partitionStorage =
mock(PartitionDataStorage.class);
Review Comment:
Why don't you use test partition storage?
Fixing these tests with mocks is a nightmare. They're also really slow
##########
modules/table/src/test/java/org/apache/ignite/internal/table/distributed/StorageUpdateHandlerTest.java:
##########
@@ -104,6 +118,56 @@ void testBuildIndex() {
verify(indexes, times(2)).addIndexToWaitIfAbsent(indexId);
}
+ @Test
+ void testVacuum() {
+ PartitionDataStorage partitionStorage = createPartitionDataStorage();
+
+ StorageUpdateHandler storageUpdateHandler =
createStorageUpdateHandler(partitionStorage,
mock(TableIndexStoragesSupplier.class));
+
+ HybridTimestamp lowWatermark = new HybridTimestamp(100, 100);
+
+ assertFalse(storageUpdateHandler.vacuum(lowWatermark));
+ verify(partitionStorage).pollForVacuum(lowWatermark);
+ // Let's check that StorageUpdateHandler#vacuumBatch returns true.
+ clearInvocations(partitionStorage);
+
+ BinaryRowAndRowId binaryRowAndRowId = new
BinaryRowAndRowId(mock(BinaryRow.class), new RowId(PARTITION_ID));
+
+
when(partitionStorage.scanVersions(any(RowId.class))).thenReturn(CursorUtils.emptyCursor());
+
when(partitionStorage.pollForVacuum(lowWatermark)).thenReturn(binaryRowAndRowId);
+
+ assertTrue(storageUpdateHandler.vacuum(lowWatermark));
+ verify(partitionStorage).pollForVacuum(lowWatermark);
+ }
+
+ @Test
+ void testExecuteBatchGc() {
+ PartitionDataStorage partitionStorage = createPartitionDataStorage();
+
+ StorageUpdateHandler storageUpdateHandler =
createStorageUpdateHandler(partitionStorage,
mock(TableIndexStoragesSupplier.class));
+
+ // Let's check that if lwm is {@code null} then nothing will happen.
+ storageUpdateHandler.executeBatchGc(null);
+
+ verify(partitionStorage,
never()).pollForVacuum(any(HybridTimestamp.class));
+
+ // Let's check that if lvm is greater than the safe time, then nothing
will happen.
+ safeTimeTracker.update(new HybridTimestamp(10, 10));
+
+ storageUpdateHandler.executeBatchGc(new HybridTimestamp(11, 1));
Review Comment:
I guess we have 2 separate APIs for GC, for partition listener and for
regular background GC. Is that correct?
If it is correct, then why?
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/raft/PartitionListener.java:
##########
@@ -88,25 +89,31 @@ public class PartitionListener implements RaftGroupListener
{
/** Storage index tracker. */
private final PendingComparableValuesTracker<Long> storageIndexTracker;
+ /** Low watermark supplier, will return {@code null} if no low watermark
has been assigned yet. */
+ private final Supplier<HybridTimestamp> lowWatermarkSupplier;
+
/**
* The constructor.
*
* @param partitionDataStorage The storage.
* @param safeTime Safe time tracker.
* @param storageIndexTracker Storage index tracker.
+ * @param lowWatermarkSupplier Low watermark supplier, will return {@code
null} if no low watermark has been assigned yet.
*/
public PartitionListener(
PartitionDataStorage partitionDataStorage,
StorageUpdateHandler storageUpdateHandler,
TxStateStorage txStateStorage,
PendingComparableValuesTracker<HybridTimestamp> safeTime,
- PendingComparableValuesTracker<Long> storageIndexTracker
+ PendingComparableValuesTracker<Long> storageIndexTracker,
+ Supplier<HybridTimestamp> lowWatermarkSupplier
Review Comment:
Can we stop using suppliers? It's annoying. When seeing `() -> null` in
constructor calls, I don't understand what it is.
Why can't we have a dedicated component/interface, that would provide the
access to LW?
##########
modules/table/src/test/java/org/apache/ignite/internal/table/distributed/IndexBaseTest.java:
##########
@@ -129,7 +130,8 @@ void setUp(@InjectConfiguration DataStorageConfiguration
dsCfg) {
PARTITION_ID,
new TestPartitionDataStorage(storage),
DummyInternalTableImpl.createTableIndexStoragesSupplier(indexes),
- dsCfg
+ dsCfg,
+ new PendingComparableValuesTracker<>(HybridTimestamp.MAX_VALUE)
Review Comment:
Same here, it's not obvious why you've chosen the max value.
And another question second time - are you sure that adding all this stuff
to storage update handler is a good idea?
--
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]
