adelapena commented on code in PR #2649:
URL: https://github.com/apache/cassandra/pull/2649#discussion_r1317196326
##########
src/java/org/apache/cassandra/db/guardrails/Guardrails.java:
##########
@@ -184,6 +184,15 @@ public final class Guardrails implements GuardrailsMBean
state ->
CONFIG_PROVIDER.getOrCreate(state).getDropKeyspaceEnabled(),
"DROP KEYSPACE functionality");
+ /**
+ * Guardrail disabling bulk loading of SSTables
+ */
+ public static final EnableFlag bulkLoadEnabled =
+ new EnableFlag("bulk_load_enabled",
+ null,
+ state ->
CONFIG_PROVIDER.getOrCreate(state).getBulkLoadEnabled(),
+ "bulk loading of SSTables");
Review Comment:
```suggestion
"Bulk loading of SSTables");
```
##########
src/java/org/apache/cassandra/config/GuardrailsOptions.java:
##########
@@ -364,6 +364,19 @@ public void setDropKeyspaceEnabled(boolean enabled)
x -> config.drop_keyspace_enabled = x);
}
+ public boolean getBulkLoadEnabled()
Review Comment:
Nit: add `@Override`
##########
src/java/org/apache/cassandra/db/guardrails/GuardrailsMBean.java:
##########
@@ -311,6 +311,18 @@ public interface GuardrailsMBean
*/
boolean getDropKeyspaceEnabled();
+ /**
+ * Sets whether bulk load of SSTables is allowed
+ */
+ void setBulkLoadEnabled(boolean enabled);
Review Comment:
The setter/getters for `bulk_load_enabled` are inserted between the getters
and setters for `drop_keyspace_enabled`, setters and getters for the same
property should remain together. Also, on this file we usually write first the
getter, then the setter.
##########
src/java/org/apache/cassandra/db/guardrails/Guardrails.java:
##########
@@ -184,6 +184,15 @@ public final class Guardrails implements GuardrailsMBean
state ->
CONFIG_PROVIDER.getOrCreate(state).getDropKeyspaceEnabled(),
"DROP KEYSPACE functionality");
+ /**
+ * Guardrail disabling bulk loading of SSTables
+ */
+ public static final EnableFlag bulkLoadEnabled =
+ new EnableFlag("bulk_load_enabled",
+ null,
Review Comment:
Can we add a reason for disabling bulk loading instead of `null`?
##########
src/java/org/apache/cassandra/streaming/StreamDeserializingTask.java:
##########
@@ -109,6 +110,10 @@ public StreamSession deriveSession(StreamMessage message)
// Attach this channel to the session: this only happens upon
receiving the first init message as a follower;
// in all other cases, no new control channel will be added, as the
proper control channel will be already attached.
streamSession.attachInbound(channel);
+
+ if (streamSession.getStreamOperation() == StreamOperation.BULK_LOAD &&
!Guardrails.bulkLoadEnabled.isEnabled(null))
+ throw new StreamReceiveException(streamSession, new
RuntimeException("Bulk load is disabled"));
Review Comment:
This isn't a proper usage of guardrails. They should be checked with
`ensureEnabled()`, so they throw the formatted `GuardrailViolatedException`
defined in their constructor. That would also trigger other associated
behaviours, like emitting a log message and a diagnostic event.
Instead, the usage of `EnableFlag#isEnabled` is ignoring all that and using
it as a regular boolean config property.
An issue here might be that we don't have a `ClientState`, and guardrails
don't throw an exception but a warning on `null` client states:
https://github.com/apache/cassandra/blob/trunk/src/java/org/apache/cassandra/db/guardrails/Guardrail.java#L139-L140
I think we could add a new `Guardrail#throwOnNullClientState()` method to
instruct guardrails to throw an exception when triggered with a null
`ClientState`. We usually don't throw on null client states to not break
background processes such as compaction, but it can make sense for
unauthenticated tools like this one.
--
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]
