ibessonov commented on code in PR #7109:
URL: https://github.com/apache/ignite-3/pull/7109#discussion_r2584561680
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/gc/GcUpdateHandler.java:
##########
@@ -130,46 +133,32 @@ private VacuumResult internalVacuumBatch(HybridTimestamp
lowWatermark, IntHolder
});
}
- private VacuumResult internalVacuum(HybridTimestamp lowWatermark, Locker
locker, boolean useTryLock) {
- while (true) {
- // Check if the storage engine needs resources before continuing.
- if (locker.shouldRelease()) {
- return VacuumResult.SHOULD_RELEASE;
- }
-
- GcEntry gcEntry = storage.peek(lowWatermark);
-
- if (gcEntry == null) {
- return VacuumResult.NO_GARBAGE_LEFT;
- }
-
- RowId rowId = gcEntry.getRowId();
+ private VacuumResult internalVacuum(GcEntry gcEntry, Locker locker,
boolean useTryLock) {
+ RowId rowId = gcEntry.getRowId();
- if (useTryLock) {
- if (!locker.tryLock(rowId)) {
- return VacuumResult.FAILED_ACQUIRE_LOCK;
- }
- } else {
- locker.lock(rowId);
+ if (useTryLock) {
+ if (!locker.tryLock(rowId)) {
+ return VacuumResult.FAILED_ACQUIRE_LOCK;
}
+ } else {
+ locker.lock(rowId);
+ }
- BinaryRow binaryRow = storage.vacuum(gcEntry);
-
- if (binaryRow == null) {
- // Removed by another thread, let's try to take another.
- continue;
- }
+ BinaryRow binaryRow = storage.vacuum(gcEntry);
- try (Cursor<ReadResult> cursor = storage.scanVersions(rowId)) {
- // TODO: IGNITE-21005 We need to choose only those indexes
that are not available for transactions
- indexUpdateHandler.tryRemoveFromIndexes(binaryRow, rowId,
cursor, null);
- }
+ if (binaryRow == null) {
+ return VacuumResult.REMOVED_BY_ANOTHER_THREAD;
Review Comment:
Does the new code rely on the fact that no GC will happen concurrently for
the same partition?
Is this the reason why you acquire RowId locks outside of `runConsistenctly`
closure (see my other comment)?
I need to understand why your code works and doesn't break anything, and
this particular detail is important for that understanding. Let's figure it out.
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/PersistentPageMemoryMvPartitionStorage.java:
##########
@@ -586,4 +590,32 @@ public boolean shouldRelease() {
return checkpointTimeoutLock.shouldReleaseReadLock();
}
}
+
+ /**
+ * This optimization reduces the IO operations performed when executing
{@link #vacuum} inside {@link #runConsistently}, which is
+ * currently executed in a loop. This will allow the checkpoint to acquire
a write lock more quickly.
+ */
+ @Override
+ protected void preloadingForGcIfNeededBusy(GcRowVersion gcRowVersion) {
+ RowId rowId = gcRowVersion.getRowId();
+ HybridTimestamp timestamp = gcRowVersion.getTimestamp();
+
+ var preloadingForGc = new PreloadingForGcInvokeClosure(rowId,
timestamp, gcRowVersion.getLink(), this);
+
+ lockByRowId.lock(rowId);
Review Comment:
Could you please explain why this is necessary?
--
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]
