You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "ArafatKhan2198 (via GitHub)" <gi...@apache.org> on 2024/04/10 10:16:47 UTC
[PR] HDDS-10614. Recon fails to start with used space cannot be negative. [ozone]
ArafatKhan2198 opened a new pull request, #6508:
URL: https://github.com/apache/ozone/pull/6508
## What changes were proposed in this pull request?
The root cause seems to be an error during the refresh operation of the `CachingSpaceUsageSource`. Specifically, the underlying `SpaceUsageSource` (likely an instance of DU, which uses the Unix du command to calculate disk usage) is failing due to a permission issue when trying to read the /data3/lost+found directory. This failure might cause the `getUsedSpace()` method to return an incorrect value (possibly zero), which, when decremented, results in a negative value. Here are the steps that are likely leading to the issue:
This PR introduces error handling and validation in the `CachingSpaceUsageSource` class to ensure data integrity. Specifically, it prevents negative values for used space by validating new values before updating the cache and handles exceptions, including `UncheckedIOException`, by maintaining the last known good value and logging errors. These changes ensure that temporary issues, such as permission errors, do not result in invalid state transitions or data corruption.
We catch `UncheckedIOException` because it indicates a problem occurred when the program tried to read or write data, and we saw it during operations like calculating disk space usage. This specific exception wraps lower-level errors, making it a clear sign that something went wrong with I/O operations, which are crucial for accurately tracking disk space.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-10614
## How was this patch tested?
CI ran green :- https://github.com/ArafatKhan2198/ozone/actions/runs/8627744703
Will be adding Unit tests for it if the approach is correct
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10614. Recon fails to start with used space cannot be negative. [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #6508:
URL: https://github.com/apache/ozone/pull/6508#discussion_r1567441714
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/fs/CachingSpaceUsageSource.java:
##########
@@ -94,7 +95,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);
+ }
Review Comment:
Please use `updateAndGet` to atomically get and set the value. Otherwise concurrent writes may update to the wrong value.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10614. Recon fails to start with used space cannot be negative. [ozone]
Posted by "ArafatKhan2198 (via GitHub)" <gi...@apache.org>.
ArafatKhan2198 commented on code in PR #6508:
URL: https://github.com/apache/ozone/pull/6508#discussion_r1568267569
##########
hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/fs/TestCachingSpaceUsageSource.java:
##########
@@ -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");
Review Comment:
Right missed it!
Thanks!
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10614. Recon fails to start with used space cannot be negative. [ozone]
Posted by "ArafatKhan2198 (via GitHub)" <gi...@apache.org>.
ArafatKhan2198 commented on code in PR #6508:
URL: https://github.com/apache/ozone/pull/6508#discussion_r1567710680
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/fs/CachingSpaceUsageSource.java:
##########
@@ -94,7 +95,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) {
Review Comment:
What my thought was that retaining the previous value can be more stable in terms of integrity, as it avoids making a potentially incorrect assumption about the space usage. If the decrement was incorrect due to a temporary error or miscalculation, keeping the previous value ensures that once the error is corrected, your cached value remains as accurate as possible.
But I do agree with you on setting to zero when an operation would make the value negative is generally safer and simpler where, regardless of input errors, the system maintains a coherent state.
Thanks for the comment @devmadhuu Ill change the code to set the value to 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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10614. Recon fails to start with used space cannot be negative. [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #6508:
URL: https://github.com/apache/ozone/pull/6508#discussion_r1568234171
##########
hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/fs/TestCachingSpaceUsageSource.java:
##########
@@ -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");
Review Comment:
Looks like this assertion needs to be updated:
```
Error: TestCachingSpaceUsageSource.testDecrementDoesNotGoNegative Time elapsed: 0.028 s <<< FAILURE!
AssertionFailedError: Cached used space should remain at the initial value ==> expected: <50> but was: <0>
```
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10614. Recon fails to start with used space cannot be negative. [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #6508:
URL: https://github.com/apache/ozone/pull/6508#issuecomment-2065039470
Thanks @ArafatKhan2198 for the fix, @devmadhuu for the review.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10614. Recon fails to start with used space cannot be negative. [ozone]
Posted by "ArafatKhan2198 (via GitHub)" <gi...@apache.org>.
ArafatKhan2198 commented on code in PR #6508:
URL: https://github.com/apache/ozone/pull/6508#discussion_r1567186147
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/fs/CachingSpaceUsageSource.java:
##########
@@ -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);
Review Comment:
I have removed it thanks!
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10614. Recon fails to start with used space cannot be negative. [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #6508:
URL: https://github.com/apache/ozone/pull/6508#discussion_r1569070957
##########
hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/fs/TestCachingSpaceUsageSource.java:
##########
@@ -142,6 +142,21 @@ public void savesValueOnShutdown() {
verify(executor).shutdown();
}
+ @Test
+ public void testDecrementDoesNotGoNegative() {
+ SpaceUsageCheckParams params = paramsBuilder(new AtomicLong(50))
+ .withRefresh(Duration.ZERO)
+ .build();
+ CachingSpaceUsageSource subject = new CachingSpaceUsageSource(params);
+ AtomicLong cachedValue = new AtomicLong(50);
Review Comment:
```
H D DLS: Dead store to cachedValue in org.apache.hadoop.hdds.fs.TestCachingSpaceUsageSource.testDecrementDoesNotGoNegative() At TestCachingSpaceUsageSource.java:[line 151]
```
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10614. Recon fails to start with used space cannot be negative. [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #6508:
URL: https://github.com/apache/ozone/pull/6508#discussion_r1570922410
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/fs/CachingSpaceUsageSource.java:
##########
@@ -94,7 +94,19 @@ public void incrementUsedSpace(long usedSpace) {
}
public void decrementUsedSpace(long reclaimedSpace) {
- cachedValue.addAndGet(-1 * reclaimedSpace);
+ cachedValue.updateAndGet(current -> {
+ long newValue = current - reclaimedSpace;
+ if (newValue < 0) {
+ if (current > 0) {
+ LOG.warn("Attempted to decrement used space to a negative value. " +
+ "Current: {}, Decrement: {}, Source: {}",
+ current, reclaimedSpace, source.toString());
Review Comment:
```suggestion
current, reclaimedSpace, source);
```
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10614. Recon fails to start with used space cannot be negative. [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #6508:
URL: https://github.com/apache/ozone/pull/6508#issuecomment-2064487968
@ArafatKhan2198 @devmadhuu This PR hopefully fixes the source of negative space at Datanode. Does Recon need any additional fix? Is space usage stored in Recon DB, which might prevent startup if Recon already saved invalid value?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10614. Avoid decreasing cached space usage below zero [ozone]
Posted by "ArafatKhan2198 (via GitHub)" <gi...@apache.org>.
ArafatKhan2198 commented on PR #6508:
URL: https://github.com/apache/ozone/pull/6508#issuecomment-2066370299
> Thanks @ArafatKhan2198 for the fix, @devmadhuu for the review.
@adoroszlai
`ClusterStateEndpoint` in Recon requests the cluster-wide storage statistics, the `ReconNodeManager` aggregates the individual Datanode statistics to calculate the `total capacity`, `used space`, and `remaining space` for the entire cluster.
The `ReconNodeManager` exposes methods like `getStats()`, which returns an object (e.g., SCMNodeStat) containing the aggregated storage statistics. The ClusterStateEndpoint then uses these aggregated statistics to create the `DatanodeStorageReport` and include it in the ClusterStateResponse.
So, in summary, the Recon component gets the storage information from the periodic heartbeat messages sent by the individual Datanodes in the cluster. The ReconNodeManager aggregates these individual Datanode statistics to provide the cluster-wide storage information to other components like the ClusterStateEndpoint.
This is my understanding from the code. We do not store the space in any of the DB's in recon (Derby or Rocks)
@devmadhuu @adoroszlai please correct me if I am making a wrong assumption anywhere.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10614. Recon fails to start with used space cannot be negative. [ozone]
Posted by "ArafatKhan2198 (via GitHub)" <gi...@apache.org>.
ArafatKhan2198 commented on code in PR #6508:
URL: https://github.com/apache/ozone/pull/6508#discussion_r1567718645
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/fs/CachingSpaceUsageSource.java:
##########
@@ -94,7 +95,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);
+ }
Review Comment:
Thanks for the idea, I have made the required changes
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10614. Recon fails to start with used space cannot be negative. [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #6508:
URL: https://github.com/apache/ozone/pull/6508#discussion_r1568763825
##########
hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/fs/TestCachingSpaceUsageSource.java:
##########
@@ -142,6 +143,24 @@ 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 and has been reset to 0
+ assertTrue(subject.getUsedSpace() >= 0,
Review Comment:
> Check ... has been reset to 0
We expect exactly 0, so please use `assertEquals(0, subject.getUsedSpace())`.
##########
hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/fs/TestCachingSpaceUsageSource.java:
##########
@@ -142,6 +143,24 @@ public void savesValueOnShutdown() {
verify(executor).shutdown();
}
+ @Test
+ public void testDecrementDoesNotGoNegative() {
+ CachingSpaceUsageSource subject;
+ SpaceUsageCheckParams params;
+ AtomicLong cachedValue;
Review Comment:
nit: please join declaration with assignment.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10614. Recon fails to start with used space cannot be negative. [ozone]
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on code in PR #6508:
URL: https://github.com/apache/ozone/pull/6508#discussion_r1567445643
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/fs/CachingSpaceUsageSource.java:
##########
@@ -94,7 +95,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) {
Review Comment:
Since usedSpace value can be decremented till ZERO and beyond that if we try to decrement, it may go negative, so this logic also not solving the problem completely. E.g. lets say we have current used space as 10 and `reclaimedSpace` is 15, ideally this should not happen, but our logic should be more resilient, in this scenario, we still should set the current used space value to ZERO because going negative is not allowed.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10614. Recon fails to start with used space cannot be negative. [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #6508:
URL: https://github.com/apache/ozone/pull/6508#discussion_r1559245486
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/fs/CachingSpaceUsageSource.java:
##########
@@ -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();
Review Comment:
`refresh()` has already been keeping the original value if `getUsedSpace()` throws exception. So the patch cannot fix negative space. I think validation should be added in `decrementUsedSpace()` instead.
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/fs/CachingSpaceUsageSource.java:
##########
@@ -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);
Review Comment:
There is no need to duplicate the `catch` block, since `UncheckedIOException` is a `RuntimeException`. (Also, if it weren't, a single clause could `catch` multiple exceptions.)
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10614. Recon fails to start with used space cannot be negative. [ozone]
Posted by "ArafatKhan2198 (via GitHub)" <gi...@apache.org>.
ArafatKhan2198 commented on PR #6508:
URL: https://github.com/apache/ozone/pull/6508#issuecomment-2047138693
@devmadhuu @adoroszlai Could you place take a look
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10614. Recon fails to start with used space cannot be negative. [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai merged PR #6508:
URL: https://github.com/apache/ozone/pull/6508
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10614. Recon fails to start with used space cannot be negative. [ozone]
Posted by "ArafatKhan2198 (via GitHub)" <gi...@apache.org>.
ArafatKhan2198 commented on code in PR #6508:
URL: https://github.com/apache/ozone/pull/6508#discussion_r1569369044
##########
hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/fs/TestCachingSpaceUsageSource.java:
##########
@@ -142,6 +142,21 @@ public void savesValueOnShutdown() {
verify(executor).shutdown();
}
+ @Test
+ public void testDecrementDoesNotGoNegative() {
+ SpaceUsageCheckParams params = paramsBuilder(new AtomicLong(50))
+ .withRefresh(Duration.ZERO)
+ .build();
+ CachingSpaceUsageSource subject = new CachingSpaceUsageSource(params);
+ AtomicLong cachedValue = new AtomicLong(50);
Review Comment:
Fixed!
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10614. Recon fails to start with used space cannot be negative. [ozone]
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on code in PR #6508:
URL: https://github.com/apache/ozone/pull/6508#discussion_r1560399439
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/fs/CachingSpaceUsageSource.java:
##########
@@ -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();
Review Comment:
Agree.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10614. Recon fails to start with used space cannot be negative. [ozone]
Posted by "ArafatKhan2198 (via GitHub)" <gi...@apache.org>.
ArafatKhan2198 commented on code in PR #6508:
URL: https://github.com/apache/ozone/pull/6508#discussion_r1567185401
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/fs/CachingSpaceUsageSource.java:
##########
@@ -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();
Review Comment:
Yes @adoroszlai , you are correct. Upon reviewing the error logs, I noticed the refresh method. Initially, I assumed that `SpaceUsageSource` might return a negative value. However, upon further looking, I realised that it cannot return negative values, instead, it may return incorrect or zero values. Consequently, decrementing that value led to the error we encountered.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10614. Recon fails to start with used space cannot be negative. [ozone]
Posted by "ArafatKhan2198 (via GitHub)" <gi...@apache.org>.
ArafatKhan2198 commented on code in PR #6508:
URL: https://github.com/apache/ozone/pull/6508#discussion_r1569057529
##########
hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/fs/TestCachingSpaceUsageSource.java:
##########
@@ -142,6 +143,24 @@ 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 and has been reset to 0
+ assertTrue(subject.getUsedSpace() >= 0,
Review Comment:
Done!
##########
hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/fs/TestCachingSpaceUsageSource.java:
##########
@@ -142,6 +143,24 @@ public void savesValueOnShutdown() {
verify(executor).shutdown();
}
+ @Test
+ public void testDecrementDoesNotGoNegative() {
+ CachingSpaceUsageSource subject;
+ SpaceUsageCheckParams params;
+ AtomicLong cachedValue;
Review Comment:
Done!
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10614. Recon fails to start with used space cannot be negative. [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #6508:
URL: https://github.com/apache/ozone/pull/6508#discussion_r1570059798
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/fs/CachingSpaceUsageSource.java:
##########
@@ -94,7 +94,17 @@ public void incrementUsedSpace(long usedSpace) {
}
public void decrementUsedSpace(long reclaimedSpace) {
- cachedValue.addAndGet(-1 * 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);
Review Comment:
If there is an error refreshing space usage (like the permission issue in HDDS-10614), `current` will not be set to the real used space. If we then keep logging a warning for each attempt to decrement space, log will be flooded.
So I think this message should be logged only if `current > 0`.
Also, please include `source` in the message to indicate the volume it affects, similar to:
https://github.com/apache/ozone/blob/f58f3d2d2a9f07c527735ee7f73cbc7deea42e0e/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/fs/CachingSpaceUsageSource.java#L152
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org