You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "rpuch (via GitHub)" <gi...@apache.org> on 2023/06/16 10:50:23 UTC
[GitHub] [ignite-3] rpuch commented on a diff in pull request #2197: IGNITE-19737 TestMvStorageUpdateHandlerTest.testConcurrentExecuteBatchGc is flacky
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org