From 9936b6b57a785dd1c0978254a6c4eb9bb3fcebaa Mon Sep 17 00:00:00 2001 From: arafat Date: Wed, 10 Apr 2024 13:14:52 +0530 Subject: [PATCH 01/13] HDDS-10614. Recon fails to start with used space cannot be negative. --- .../hdds/fs/CachingSpaceUsageSource.java | 21 +++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/fs/CachingSpaceUsageSource.java b/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/fs/CachingSpaceUsageSource.java index 6f776072d9c3..c0876db93729 100644 --- a/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/fs/CachingSpaceUsageSource.java +++ b/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/fs/CachingSpaceUsageSource.java @@ -25,6 +25,8 @@ import org.slf4j.LoggerFactory; import jakarta.annotation.Nullable; + +import java.io.UncheckedIOException; import java.time.Duration; import java.util.OptionalLong; import java.util.concurrent.Executors; @@ -137,9 +139,24 @@ private void refresh() { //only one `refresh` can be running at a certain moment if (isRefreshRunning.compareAndSet(false, true)) { try { - cachedValue.set(source.getUsedSpace()); + long newUsedSpace = source.getUsedSpace(); + // Check for negative value before setting it to the cache + if (newUsedSpace >= 0) { + cachedValue.set(newUsedSpace); + } else { + LOG.error( + "Received negative used space value: {}. Keeping the last known good value: {}.", + newUsedSpace, cachedValue.get()); + } + } catch (UncheckedIOException e) { + // Log the error and do not update the cached value + LOG.error( + "Error refreshing space usage for {}. Keeping the last known good value: {}", + source, cachedValue.get(), e); } catch (RuntimeException e) { - LOG.warn("Error refreshing space usage for {}", source, e); + LOG.error( + "Error refreshing space usage for {}. Keeping the last known good value: {}", + source, cachedValue.get(), e); } finally { isRefreshRunning.set(false); } From 6771216917066a7b4025aa6d512653872379ffb8 Mon Sep 17 00:00:00 2001 From: arafat Date: Tue, 16 Apr 2024 16:48:36 +0530 Subject: [PATCH 02/13] Moved the negative check logic to decrementUsedSpace --- .../hdds/fs/CachingSpaceUsageSource.java | 30 ++++++++----------- 1 file changed, 12 insertions(+), 18 deletions(-) diff --git a/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/fs/CachingSpaceUsageSource.java b/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/fs/CachingSpaceUsageSource.java index c0876db93729..bf00f23bc7f4 100644 --- a/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/fs/CachingSpaceUsageSource.java +++ b/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/fs/CachingSpaceUsageSource.java @@ -96,7 +96,16 @@ public void incrementUsedSpace(long usedSpace) { } public void decrementUsedSpace(long reclaimedSpace) { - cachedValue.addAndGet(-1 * reclaimedSpace); + long current = cachedValue.get(); + long newValue = current - reclaimedSpace; + if (newValue < 0) { + LOG.warn( + "Attempted to decrement used space to a negative value. Current: {}, Decrement: {}", + current, reclaimedSpace); + cachedValue.set(current); // Retain the previous value + } else { + cachedValue.set(newValue); + } } public void start() { @@ -139,24 +148,9 @@ private void refresh() { //only one `refresh` can be running at a certain moment if (isRefreshRunning.compareAndSet(false, true)) { try { - long newUsedSpace = source.getUsedSpace(); - // Check for negative value before setting it to the cache - if (newUsedSpace >= 0) { - cachedValue.set(newUsedSpace); - } else { - LOG.error( - "Received negative used space value: {}. Keeping the last known good value: {}.", - newUsedSpace, cachedValue.get()); - } - } catch (UncheckedIOException e) { - // Log the error and do not update the cached value - LOG.error( - "Error refreshing space usage for {}. Keeping the last known good value: {}", - source, cachedValue.get(), e); + cachedValue.set(source.getUsedSpace()); } catch (RuntimeException e) { - LOG.error( - "Error refreshing space usage for {}. Keeping the last known good value: {}", - source, cachedValue.get(), e); + LOG.warn("Error refreshing space usage for {}", source, e); } finally { isRefreshRunning.set(false); } From 25e632cc0d547ed87ba0b3de86228d2f1e223fd0 Mon Sep 17 00:00:00 2001 From: arafat Date: Tue, 16 Apr 2024 16:54:48 +0530 Subject: [PATCH 03/13] Removed unused import --- .../java/org/apache/hadoop/hdds/fs/CachingSpaceUsageSource.java | 1 - 1 file changed, 1 deletion(-) diff --git a/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/fs/CachingSpaceUsageSource.java b/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/fs/CachingSpaceUsageSource.java index bf00f23bc7f4..3312217acf50 100644 --- a/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/fs/CachingSpaceUsageSource.java +++ b/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/fs/CachingSpaceUsageSource.java @@ -26,7 +26,6 @@ import jakarta.annotation.Nullable; -import java.io.UncheckedIOException; import java.time.Duration; import java.util.OptionalLong; import java.util.concurrent.Executors; From 5cc86d19243e4cf10ecfbf52eb38d73b83274307 Mon Sep 17 00:00:00 2001 From: arafat Date: Tue, 16 Apr 2024 18:11:33 +0530 Subject: [PATCH 04/13] Added Unit test --- .../hdds/fs/TestCachingSpaceUsageSource.java | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/fs/TestCachingSpaceUsageSource.java b/hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/fs/TestCachingSpaceUsageSource.java index 674c1233dee6..41cdfbee9249 100644 --- a/hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/fs/TestCachingSpaceUsageSource.java +++ b/hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/fs/TestCachingSpaceUsageSource.java @@ -31,6 +31,7 @@ import java.util.concurrent.atomic.AtomicLong; import static org.apache.hadoop.hdds.fs.MockSpaceUsageCheckParams.newBuilder; +import static org.apache.ratis.util.Preconditions.assertTrue; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.mockito.Mockito.any; import static org.mockito.Mockito.anyLong; @@ -142,6 +143,28 @@ public void savesValueOnShutdown() { verify(executor).shutdown(); } + @Test + public void testDecrementDoesNotGoNegative() { + CachingSpaceUsageSource subject; + SpaceUsageCheckParams params; + AtomicLong cachedValue; + cachedValue = new AtomicLong(50); + params = paramsBuilder(cachedValue) + .withRefresh(Duration.ZERO) + .build(); + subject = new CachingSpaceUsageSource(params); + // Try to decrement more than the current value + subject.decrementUsedSpace(100); + + // Check that the value has not gone negative + assertTrue(subject.getUsedSpace() >= 0, + "Cached used space should not be negative"); + + // Check that it has retained the previous value + assertEquals(50, subject.getUsedSpace(), + "Cached used space should remain at the initial value"); + } + private static long missingInitialValue() { return 0L; } From d0ff3f5f9586e06c44f4361876de33ad6c1a4bbd Mon Sep 17 00:00:00 2001 From: arafat Date: Tue, 16 Apr 2024 22:54:44 +0530 Subject: [PATCH 05/13] Made review comment changes --- .../hdds/fs/CachingSpaceUsageSource.java | 21 ++++++++++--------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/fs/CachingSpaceUsageSource.java b/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/fs/CachingSpaceUsageSource.java index 3312217acf50..49c2d3b65138 100644 --- a/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/fs/CachingSpaceUsageSource.java +++ b/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/fs/CachingSpaceUsageSource.java @@ -95,16 +95,17 @@ public void incrementUsedSpace(long usedSpace) { } public void decrementUsedSpace(long reclaimedSpace) { - long current = cachedValue.get(); - long newValue = current - reclaimedSpace; - if (newValue < 0) { - LOG.warn( - "Attempted to decrement used space to a negative value. Current: {}, Decrement: {}", - current, reclaimedSpace); - cachedValue.set(current); // Retain the previous value - } else { - cachedValue.set(newValue); - } + cachedValue.updateAndGet(current -> { + long newValue = current - reclaimedSpace; + if (newValue < 0) { + LOG.warn( + "Attempted to decrement used space to a negative value. Current: {}, Decrement: {}", + current, reclaimedSpace); + return 0; // Reset to zero + } else { + return newValue; + } + }); } public void start() { From b2fc96d09f0a03f8f15901b23c52358c262a8609 Mon Sep 17 00:00:00 2001 From: arafat Date: Wed, 17 Apr 2024 11:29:29 +0530 Subject: [PATCH 06/13] Fixed the failing UT --- .../apache/hadoop/hdds/fs/TestCachingSpaceUsageSource.java | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/fs/TestCachingSpaceUsageSource.java b/hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/fs/TestCachingSpaceUsageSource.java index 41cdfbee9249..7074b849ecba 100644 --- a/hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/fs/TestCachingSpaceUsageSource.java +++ b/hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/fs/TestCachingSpaceUsageSource.java @@ -156,13 +156,9 @@ public void testDecrementDoesNotGoNegative() { // Try to decrement more than the current value subject.decrementUsedSpace(100); - // Check that the value has not gone negative + // Check that the value has not gone negative and has been reset to 0 assertTrue(subject.getUsedSpace() >= 0, "Cached used space should not be negative"); - - // Check that it has retained the previous value - assertEquals(50, subject.getUsedSpace(), - "Cached used space should remain at the initial value"); } private static long missingInitialValue() { From fc06d39a358ff5819ffa4d3ebfa934513355a992 Mon Sep 17 00:00:00 2001 From: arafat Date: Wed, 17 Apr 2024 20:08:02 +0530 Subject: [PATCH 07/13] Made changes to the assertions and did some refactoring --- .../hdds/fs/TestCachingSpaceUsageSource.java | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/fs/TestCachingSpaceUsageSource.java b/hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/fs/TestCachingSpaceUsageSource.java index 7074b849ecba..604b153b0324 100644 --- a/hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/fs/TestCachingSpaceUsageSource.java +++ b/hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/fs/TestCachingSpaceUsageSource.java @@ -145,20 +145,17 @@ public void savesValueOnShutdown() { @Test public void testDecrementDoesNotGoNegative() { - CachingSpaceUsageSource subject; - SpaceUsageCheckParams params; - AtomicLong cachedValue; - cachedValue = new AtomicLong(50); - params = paramsBuilder(cachedValue) + SpaceUsageCheckParams params = paramsBuilder(new AtomicLong(50)) .withRefresh(Duration.ZERO) .build(); - subject = new CachingSpaceUsageSource(params); + CachingSpaceUsageSource subject = new CachingSpaceUsageSource(params); + AtomicLong cachedValue = new AtomicLong(50); + // Try to decrement more than the current value subject.decrementUsedSpace(100); - // Check that the value has not gone negative and has been reset to 0 - assertTrue(subject.getUsedSpace() >= 0, - "Cached used space should not be negative"); + // Check that the value has been set to 0 + assertEquals(0, subject.getUsedSpace()); } private static long missingInitialValue() { From d341102b86701598535a64b873c2088091a1bb49 Mon Sep 17 00:00:00 2001 From: arafat Date: Wed, 17 Apr 2024 20:14:31 +0530 Subject: [PATCH 08/13] Fixed Checkstyle --- .../org/apache/hadoop/hdds/fs/TestCachingSpaceUsageSource.java | 1 - 1 file changed, 1 deletion(-) diff --git a/hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/fs/TestCachingSpaceUsageSource.java b/hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/fs/TestCachingSpaceUsageSource.java index 604b153b0324..0e7cbaca8c31 100644 --- a/hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/fs/TestCachingSpaceUsageSource.java +++ b/hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/fs/TestCachingSpaceUsageSource.java @@ -31,7 +31,6 @@ import java.util.concurrent.atomic.AtomicLong; import static org.apache.hadoop.hdds.fs.MockSpaceUsageCheckParams.newBuilder; -import static org.apache.ratis.util.Preconditions.assertTrue; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.mockito.Mockito.any; import static org.mockito.Mockito.anyLong; From 05081d69d6c08795ba0b61181c5067c7f43362a8 Mon Sep 17 00:00:00 2001 From: arafat Date: Wed, 17 Apr 2024 21:49:32 +0530 Subject: [PATCH 09/13] Fixed bug --- .../org/apache/hadoop/hdds/fs/TestCachingSpaceUsageSource.java | 1 - 1 file changed, 1 deletion(-) diff --git a/hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/fs/TestCachingSpaceUsageSource.java b/hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/fs/TestCachingSpaceUsageSource.java index 0e7cbaca8c31..8523861000e7 100644 --- a/hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/fs/TestCachingSpaceUsageSource.java +++ b/hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/fs/TestCachingSpaceUsageSource.java @@ -148,7 +148,6 @@ public void testDecrementDoesNotGoNegative() { .withRefresh(Duration.ZERO) .build(); CachingSpaceUsageSource subject = new CachingSpaceUsageSource(params); - AtomicLong cachedValue = new AtomicLong(50); // Try to decrement more than the current value subject.decrementUsedSpace(100); From f58f3d2d2a9f07c527735ee7f73cbc7deea42e0e Mon Sep 17 00:00:00 2001 From: arafat Date: Thu, 18 Apr 2024 00:45:27 +0530 Subject: [PATCH 10/13] Removed unnecessary change --- .../java/org/apache/hadoop/hdds/fs/CachingSpaceUsageSource.java | 1 - 1 file changed, 1 deletion(-) diff --git a/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/fs/CachingSpaceUsageSource.java b/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/fs/CachingSpaceUsageSource.java index 49c2d3b65138..f3f0ebddb87d 100644 --- a/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/fs/CachingSpaceUsageSource.java +++ b/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/fs/CachingSpaceUsageSource.java @@ -25,7 +25,6 @@ import org.slf4j.LoggerFactory; import jakarta.annotation.Nullable; - import java.time.Duration; import java.util.OptionalLong; import java.util.concurrent.Executors; From 8f5615834631c4affcefa8cdd33fcbf222da8c50 Mon Sep 17 00:00:00 2001 From: arafat Date: Thu, 18 Apr 2024 19:40:19 +0530 Subject: [PATCH 11/13] Added condition for logging --- .../apache/hadoop/hdds/fs/CachingSpaceUsageSource.java | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/fs/CachingSpaceUsageSource.java b/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/fs/CachingSpaceUsageSource.java index f3f0ebddb87d..05d3a4f88a8d 100644 --- a/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/fs/CachingSpaceUsageSource.java +++ b/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/fs/CachingSpaceUsageSource.java @@ -97,10 +97,11 @@ public void decrementUsedSpace(long reclaimedSpace) { cachedValue.updateAndGet(current -> { long newValue = current - reclaimedSpace; if (newValue < 0) { - LOG.warn( - "Attempted to decrement used space to a negative value. Current: {}, Decrement: {}", - current, reclaimedSpace); - return 0; // Reset to zero + if (current > 0) { + LOG.warn("Attempted to decrement used space to a negative value. " + + "Current: {}, Decrement: {}", current, reclaimedSpace); + } + return 0; } else { return newValue; } From 04e9dcf119230754f8c64f9df46e5bb417a01698 Mon Sep 17 00:00:00 2001 From: arafat Date: Thu, 18 Apr 2024 20:04:00 +0530 Subject: [PATCH 12/13] Added the data volume also to the log message --- .../org/apache/hadoop/hdds/fs/CachingSpaceUsageSource.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/fs/CachingSpaceUsageSource.java b/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/fs/CachingSpaceUsageSource.java index 05d3a4f88a8d..205be342fc3e 100644 --- a/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/fs/CachingSpaceUsageSource.java +++ b/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/fs/CachingSpaceUsageSource.java @@ -99,7 +99,8 @@ public void decrementUsedSpace(long reclaimedSpace) { if (newValue < 0) { if (current > 0) { LOG.warn("Attempted to decrement used space to a negative value. " + - "Current: {}, Decrement: {}", current, reclaimedSpace); + "Current: {}, Decrement: {}, Source: {}", + current, reclaimedSpace, source.toString()); } return 0; } else { From 32c3809677d3c099c5df9a2db0d91c264b9a64a0 Mon Sep 17 00:00:00 2001 From: "Doroszlai, Attila" <6454655+adoroszlai@users.noreply.github.com> Date: Thu, 18 Apr 2024 16:53:59 +0200 Subject: [PATCH 13/13] Remove unnecessary toString --- .../java/org/apache/hadoop/hdds/fs/CachingSpaceUsageSource.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/fs/CachingSpaceUsageSource.java b/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/fs/CachingSpaceUsageSource.java index 205be342fc3e..b9a2f87a03da 100644 --- a/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/fs/CachingSpaceUsageSource.java +++ b/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/fs/CachingSpaceUsageSource.java @@ -100,7 +100,7 @@ public void decrementUsedSpace(long reclaimedSpace) { if (current > 0) { LOG.warn("Attempted to decrement used space to a negative value. " + "Current: {}, Decrement: {}, Source: {}", - current, reclaimedSpace, source.toString()); + current, reclaimedSpace, source); } return 0; } else {