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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
