rpuch commented on code in PR #2197:
URL: https://github.com/apache/ignite-3/pull/2197#discussion_r1232070332
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/gc/GcUpdateHandler.java:
##########
@@ -69,41 +68,74 @@ public PendingComparableValuesTracker<HybridTimestamp,
Void> getSafeTimeTracker(
*
* @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) {
+ public boolean vacuumBatch(HybridTimestamp lowWatermark, int count,
boolean strict) {
+ if (count <= 0) {
+ return true;
+ }
+
+ IntHolder countHolder = new IntHolder(count);
+
+ while (countHolder.get() > 0) {
+ VacuumResult vacuumResult = internalVacuumBatch(lowWatermark,
countHolder);
+
+ switch (vacuumResult) {
+ case NO_GARBAGE_LEFT:
+ return false;
+ case SUCCESS:
+ return true;
+ case FAILED_ACQUIRE_LOCK:
+ if (strict) {
+ continue;
+ }
+
+ return true;
+ default:
+ throw new IllegalStateException(vacuumResult.toString());
+ }
+ }
+
+ return true;
+ }
+
+ private VacuumResult internalVacuumBatch(HybridTimestamp lowWatermark,
IntHolder countHolder) {
return storage.runConsistently(locker -> {
+ int count = countHolder.get();
+
for (int i = 0; i < count; i++) {
- if (!internalVacuum(lowWatermark, locker)) {
- return false;
+ // It is safe for the first iteration to use a lock instead of
tryLock.
+ VacuumResult vacuumResult = internalVacuum(lowWatermark,
locker, i > 0);
+
+ if (vacuumResult != VacuumResult.SUCCESS) {
+ return vacuumResult;
}
+
+ countHolder.getAndDecrement();
}
- return true;
+ return VacuumResult.SUCCESS;
});
}
- /**
- * Attempts to collect garbage for one {@link RowId}.
- *
- * <p>Must be called inside a {@link
PartitionDataStorage#runConsistently(WriteClosure)} closure.
- *
- * @param lowWatermark Low watermark for the vacuum.
- * @param locker From {@link
PartitionDataStorage#runConsistently(WriteClosure)}.
- * @return {@code False} if there is no garbage left in the {@link
#storage}.
- */
- private boolean internalVacuum(HybridTimestamp lowWatermark, Locker
locker) {
+ private VacuumResult internalVacuum(HybridTimestamp lowWatermark, Locker
locker, boolean useTryLock) {
while (true) {
GcEntry gcEntry = storage.peek(lowWatermark);
if (gcEntry == null) {
- return false;
+ return VacuumResult.NO_GARBAGE_LEFT;
}
RowId rowId = gcEntry.getRowId();
- if (!locker.tryLock(rowId)) {
- return true;
+ if (!useTryLock) {
Review Comment:
How about inverting the condition (and swapping the branches)? It's a bit
easier to understand a condition without a negation
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/gc/GcUpdateHandler.java:
##########
@@ -69,41 +68,74 @@ public PendingComparableValuesTracker<HybridTimestamp,
Void> getSafeTimeTracker(
*
* @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) {
+ public boolean vacuumBatch(HybridTimestamp lowWatermark, int count,
boolean strict) {
+ if (count <= 0) {
+ return true;
+ }
+
+ IntHolder countHolder = new IntHolder(count);
+
+ while (countHolder.get() > 0) {
+ VacuumResult vacuumResult = internalVacuumBatch(lowWatermark,
countHolder);
+
+ switch (vacuumResult) {
+ case NO_GARBAGE_LEFT:
+ return false;
+ case SUCCESS:
+ return true;
+ case FAILED_ACQUIRE_LOCK:
+ if (strict) {
+ continue;
+ }
+
+ return true;
+ default:
+ throw new IllegalStateException(vacuumResult.toString());
+ }
+ }
+
+ return true;
+ }
+
+ private VacuumResult internalVacuumBatch(HybridTimestamp lowWatermark,
IntHolder countHolder) {
return storage.runConsistently(locker -> {
+ int count = countHolder.get();
+
for (int i = 0; i < count; i++) {
- if (!internalVacuum(lowWatermark, locker)) {
- return false;
+ // It is safe for the first iteration to use a lock instead of
tryLock.
Review Comment:
Why is it safe specifically for the first iteration?
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/gc/GcUpdateHandler.java:
##########
@@ -69,41 +68,74 @@ public PendingComparableValuesTracker<HybridTimestamp,
Void> getSafeTimeTracker(
*
* @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) {
+ public boolean vacuumBatch(HybridTimestamp lowWatermark, int count,
boolean strict) {
+ if (count <= 0) {
+ return true;
+ }
+
+ IntHolder countHolder = new IntHolder(count);
Review Comment:
It looks like `IntHolder` is always used for counting down. How about
renaming it to `CountDown`? It could also have a method like `finished()` that
would check that the count reached zero
--
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]
