You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "isaacreath (via GitHub)" <gi...@apache.org> on 2025/12/05 17:47:28 UTC
[PR] CASSANDRA-21057: Disk usage guardrail cannot be disabled when failure threshold is reached [cassandra]
isaacreath opened a new pull request, #4515:
URL: https://github.com/apache/cassandra/pull/4515
This patch updates the DiskUsageMonitor to set local disk usage state to `NOT_AVAILABLE` if the disk usage guardrails are disabled & adds a dtest to validate this behavior.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-21057: [trunk] Disk usage guardrail cannot be disabled when failure threshold is reached [cassandra]
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #4515:
URL: https://github.com/apache/cassandra/pull/4515#discussion_r2594224585
##########
src/java/org/apache/cassandra/service/disk/usage/DiskUsageMonitor.java:
##########
@@ -79,14 +81,28 @@ public void start(Consumer<DiskUsageState> notifier)
{
// start the scheduler regardless guardrail is enabled, so we can enable it later without a restart
ScheduledExecutors.scheduledTasks.scheduleAtFixedRate(() -> {
-
- if (!Guardrails.localDataDiskUsage.enabled(null))
+ boolean currentEnabledStatus = Guardrails.localDataDiskUsage.enabled(null);
+ boolean oldEnabledStatus = enabled;
+ enabled = currentEnabledStatus;
+ if (!currentEnabledStatus)
Review Comment:
this `if` is ever entered only in case `currentEnabledStatus` is `false`.
Then you do
if (currentEnabledStatus != oldEnabledStatus)
That means that `currentEnabledStatus` is `true`, which can not equal to `oldEnbledStatus`.
That means that in order to call `onDiskUsageGuardrailDisabled` method, `oldEnabledStatus` has to be `true`.
This can then be simplified to
if (!currentEnabledStatus)
{
if (oldEnabledStatus)
onDiskUsageGuardrailDisabled(notifier);
}
else
updateLocalState(getDiskUsage(), notifier);
Which can be further simplified to
if (!currentEnabledStatus && oldEnabledStatus)
onDiskUsageGuardrailDisabled(notifier);
else
updateLocalState(getDiskUsage(), notifier);
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-21057: [trunk] Disk usage guardrail cannot be disabled when failure threshold is reached [cassandra]
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #4515:
URL: https://github.com/apache/cassandra/pull/4515#discussion_r2594224585
##########
src/java/org/apache/cassandra/service/disk/usage/DiskUsageMonitor.java:
##########
@@ -79,14 +81,28 @@ public void start(Consumer<DiskUsageState> notifier)
{
// start the scheduler regardless guardrail is enabled, so we can enable it later without a restart
ScheduledExecutors.scheduledTasks.scheduleAtFixedRate(() -> {
-
- if (!Guardrails.localDataDiskUsage.enabled(null))
+ boolean currentEnabledStatus = Guardrails.localDataDiskUsage.enabled(null);
+ boolean oldEnabledStatus = enabled;
+ enabled = currentEnabledStatus;
+ if (!currentEnabledStatus)
Review Comment:
this `if` is ever entered only in case `currentEnabledStatus` is `false`.
Then you do
if (currentEnabledStatus != oldEnabledStatus)
That means that `currentEnabledStatus is true, which can not equal to `oldEnbledStatus`.
That means that in order to call `onDiskUsageGuardrailDisabled` method, `oldEnabledStatus` has to be `true`.
This can then be simplified to
if (!currentEnabledStatus)
{
if (oldEnabledStatus)
onDiskUsageGuardrailDisabled(notifier);
}
else
updateLocalState(getDiskUsage(), notifier);
Which can be further simplified to
if (!currentEnabledStatus && oldEnabledStatus)
onDiskUsageGuardrailDisabled(notifier);
else
updateLocalState(getDiskUsage(), notifier);
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-21057: [trunk] Disk usage guardrail cannot be disabled when failure threshold is reached [cassandra]
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #4515:
URL: https://github.com/apache/cassandra/pull/4515#discussion_r2594149971
##########
src/java/org/apache/cassandra/service/disk/usage/DiskUsageMonitor.java:
##########
@@ -79,14 +81,28 @@ public void start(Consumer<DiskUsageState> notifier)
{
// start the scheduler regardless guardrail is enabled, so we can enable it later without a restart
ScheduledExecutors.scheduledTasks.scheduleAtFixedRate(() -> {
-
- if (!Guardrails.localDataDiskUsage.enabled(null))
+ boolean currentEnabledStatus = Guardrails.localDataDiskUsage.enabled(null);
Review Comment:
what about this
````
boolean oldEnabledStatus = enabled.getAndSet(Guardrails.localDataDiskUsage.enabled(null));
if (!enabled.get())
{
if (oldEnabledStatus)
onDiskUsageGuardrailDisabled(notifier);
return;
}
````
what is here is just too verbose to my taste
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-21057: [trunk] Disk usage guardrail cannot be disabled when failure threshold is reached [cassandra]
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #4515:
URL: https://github.com/apache/cassandra/pull/4515#discussion_r2594149971
##########
src/java/org/apache/cassandra/service/disk/usage/DiskUsageMonitor.java:
##########
@@ -79,14 +81,28 @@ public void start(Consumer<DiskUsageState> notifier)
{
// start the scheduler regardless guardrail is enabled, so we can enable it later without a restart
ScheduledExecutors.scheduledTasks.scheduleAtFixedRate(() -> {
-
- if (!Guardrails.localDataDiskUsage.enabled(null))
+ boolean currentEnabledStatus = Guardrails.localDataDiskUsage.enabled(null);
Review Comment:
what about this
boolean oldEnabledStatus = enabled.getAndSet(Guardrails.localDataDiskUsage.enabled(null));
if (!enabled.get())
{
if (oldEnabledStatus)
onDiskUsageGuardrailDisabled(notifier);
return;
}
what is here is just too verbose to my taste
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-21057: [trunk] Disk usage guardrail cannot be disabled when failure threshold is reached [cassandra]
Posted by "isaacreath (via GitHub)" <gi...@apache.org>.
isaacreath commented on code in PR #4515:
URL: https://github.com/apache/cassandra/pull/4515#discussion_r2595045588
##########
src/java/org/apache/cassandra/service/disk/usage/DiskUsageMonitor.java:
##########
@@ -59,11 +59,13 @@ public class DiskUsageMonitor
private final Supplier<Multimap<FileStore, Directories.DataDirectory>> dataDirectoriesSupplier;
private volatile DiskUsageState localState = DiskUsageState.NOT_AVAILABLE;
+ private volatile boolean enabled;
@VisibleForTesting
public DiskUsageMonitor()
{
this.dataDirectoriesSupplier = DiskUsageMonitor::dataDirectoriesGroupedByFileStore;
+ this.enabled = false; // Default to false. Will be set to true on the first call to `start`
Review Comment:
Yeah you're right. Oversight on my part.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-21057: [trunk] Disk usage guardrail cannot be disabled when failure threshold is reached [cassandra]
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #4515:
URL: https://github.com/apache/cassandra/pull/4515#discussion_r2594149971
##########
src/java/org/apache/cassandra/service/disk/usage/DiskUsageMonitor.java:
##########
@@ -79,14 +81,28 @@ public void start(Consumer<DiskUsageState> notifier)
{
// start the scheduler regardless guardrail is enabled, so we can enable it later without a restart
ScheduledExecutors.scheduledTasks.scheduleAtFixedRate(() -> {
-
- if (!Guardrails.localDataDiskUsage.enabled(null))
+ boolean currentEnabledStatus = Guardrails.localDataDiskUsage.enabled(null);
Review Comment:
what about this
````
boolean oldEnabledStatus = enabled.getAndSet(Guardrails.localDataDiskUsage.enabled(null));
if (!enabled.get())
{
if (oldEnabledStatus)
onDiskUsageGuardrailDisabled(notifier);
return;
}
````
what is here is just too verbose to my taste
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-21057: [trunk] Disk usage guardrail cannot be disabled when failure threshold is reached [cassandra]
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #4515:
URL: https://github.com/apache/cassandra/pull/4515#discussion_r2594224585
##########
src/java/org/apache/cassandra/service/disk/usage/DiskUsageMonitor.java:
##########
@@ -79,14 +81,28 @@ public void start(Consumer<DiskUsageState> notifier)
{
// start the scheduler regardless guardrail is enabled, so we can enable it later without a restart
ScheduledExecutors.scheduledTasks.scheduleAtFixedRate(() -> {
-
- if (!Guardrails.localDataDiskUsage.enabled(null))
+ boolean currentEnabledStatus = Guardrails.localDataDiskUsage.enabled(null);
+ boolean oldEnabledStatus = enabled;
+ enabled = currentEnabledStatus;
+ if (!currentEnabledStatus)
Review Comment:
this `if` is ever entered only in case `currentEnabledStatus` is `false`.
The you do
if (currentEnabledStatus != oldEnabledStatus)
That means that `currentEnabledStatus is true, which can not equal to `oldEnbledStatus`.
That means that in order to call `onDiskUsageGuardrailDisabled` method, `oldEnabledStatus` has to be `true`.
This can then be simplified to
if (!currentEnabledStatus)
{
if (oldEnabledStatus)
onDiskUsageGuardrailDisabled(notifier);
}
else
updateLocalState(getDiskUsage(), notifier);
Which can be further simplified to
if (!currentEnabledStatus && oldEnabledStatus)
onDiskUsageGuardrailDisabled(notifier);
else
updateLocalState(getDiskUsage(), notifier);
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-21057: [trunk] Disk usage guardrail cannot be disabled when failure threshold is reached [cassandra]
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic closed pull request #4515: CASSANDRA-21057: [trunk] Disk usage guardrail cannot be disabled when failure threshold is reached
URL: https://github.com/apache/cassandra/pull/4515
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-21057: [trunk] Disk usage guardrail cannot be disabled when failure threshold is reached [cassandra]
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #4515:
URL: https://github.com/apache/cassandra/pull/4515#discussion_r2594188302
##########
src/java/org/apache/cassandra/service/disk/usage/DiskUsageMonitor.java:
##########
@@ -59,11 +59,13 @@ public class DiskUsageMonitor
private final Supplier<Multimap<FileStore, Directories.DataDirectory>> dataDirectoriesSupplier;
private volatile DiskUsageState localState = DiskUsageState.NOT_AVAILABLE;
+ private volatile boolean enabled;
@VisibleForTesting
public DiskUsageMonitor()
{
this.dataDirectoriesSupplier = DiskUsageMonitor::dataDirectoriesGroupedByFileStore;
+ this.enabled = false; // Default to false. Will be set to true on the first call to `start`
Review Comment:
do you really need to do this? is not false default value for boolean anyway? Also, it should be moved to the below constructor and here replace this one with `this(DiskUsageMonitor::dataDirectoriesGroupedByFileStore)`
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-21057: [trunk] Disk usage guardrail cannot be disabled when failure threshold is reached [cassandra]
Posted by "jyothsnakonisa (via GitHub)" <gi...@apache.org>.
jyothsnakonisa commented on code in PR #4515:
URL: https://github.com/apache/cassandra/pull/4515#discussion_r2600090569
##########
src/java/org/apache/cassandra/service/disk/usage/DiskUsageMonitor.java:
##########
@@ -79,14 +80,22 @@ public void start(Consumer<DiskUsageState> notifier)
{
// start the scheduler regardless guardrail is enabled, so we can enable it later without a restart
ScheduledExecutors.scheduledTasks.scheduleAtFixedRate(() -> {
-
- if (!Guardrails.localDataDiskUsage.enabled(null))
- return;
-
- updateLocalState(getDiskUsage(), notifier);
+ boolean currentEnabledStatus = Guardrails.localDataDiskUsage.enabled(null);
+ boolean oldEnabledStatus = enabled;
+ enabled = currentEnabledStatus;
+ if (!currentEnabledStatus && oldEnabledStatus)
+ onDiskUsageGuardrailDisabled(notifier);
+ else
+ updateLocalState(getDiskUsage(), notifier);
Review Comment:
Optional, may be consider extracting the if condition into a variable for better readability?
```suggestion
boolean currentEnabled = Guardrails.localDataDiskUsage.enabled(null);
boolean oldEnabled = enabled;
boolean isDisabled = !currentEnabled && oldEnabled;
enabled = currentEnabled;
if (isDisabled)
onDiskUsageGuardrailDisabled(notifier);
else
updateLocalState(getDiskUsage(), notifier);
```
##########
test/distributed/org/apache/cassandra/distributed/test/guardrails/GuardrailDiskUsageTest.java:
##########
@@ -202,6 +203,73 @@ public void testDiskUsage() throws Throwable
}
}
+ @Test
+ public void testDiskUsageNodetoolDisableWhenDiskIsFullShouldEnableWrites()
+ {
+ schemaChange("CREATE TABLE %s (k int PRIMARY KEY, v int)");
+ String insert = format("INSERT INTO %s(k, v) VALUES (?, 0)");
+
+ // With both nodes in SPACIOUS state, we can write without warnings nor failures
+ for (int i = 0; i < NUM_ROWS; i++)
+ {
+ ResultSet rs = driverSession.execute(insert, i);
+ Assertions.assertThat(rs.getExecutionInfo().getWarnings()).isEmpty();
+ }
+
+ // If the STUFFED node becomes FULL, the writes targeting that node will fail, while the writes targeting
+ // the node that remains SPACIOUS will keep succeeding without warnings
+ DiskStateInjection.setState(getCluster(), 2, DiskUsageState.FULL);
+ int numFailures = 0;
+ for (int i = 0; i < NUM_ROWS; i++)
+ {
+ try
+ {
+ ResultSet rs = driverSession.execute(insert, i);
+ Assertions.assertThat(rs.getExecutionInfo().getWarnings()).isEmpty();
+ }
+ catch (InvalidQueryException e)
+ {
+ Assertions.assertThat(e).hasMessageContaining(FAIL_MESSAGE);
+ numFailures++;
+ }
+ }
+ Assertions.assertThat(numFailures).isGreaterThan(0).isLessThan(NUM_ROWS);
+
+ // After disabling the guardrail, we should be able to write again.
+ cluster.get(2).runOnInstance(() -> Guardrails.instance.setDataDiskUsagePercentageThreshold(-1, -1));
+ int stateDissemenationTimeoutSec = 2 * 60; // 2 minutes.
+ loopAssert(stateDissemenationTimeoutSec, 100, () -> {
+ Assertions.assertThat(cluster.get(1).callOnInstance(() -> !DiskUsageBroadcaster.instance.hasStuffedOrFullNode())).isTrue();
+ });
Review Comment:
Can you please see if you can avoid introducing `loopAssert` and adding new class `AssertionUtils` by using `Utils.spinUntilSuccess`. You might be able to do something like this
```suggestion
Util.spinUntilSuccess(() -> {
Assertions.assertThat(cluster.get(1).callOnInstance(() -> !DiskUsageBroadcaster.instance.hasStuffedOrFullNode())).isTrue();
}, stateDissemenationTimeoutSec);
```
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org