smiklosovic commented on code in PR #4393:
URL: https://github.com/apache/cassandra/pull/4393#discussion_r2383992357
##########
src/java/org/apache/cassandra/db/guardrails/Guardrails.java:
##########
@@ -143,6 +143,20 @@ public final class Guardrails implements GuardrailsMBean
state ->
CONFIG_PROVIDER.getOrCreate(state).getTablePropertiesDisallowed(),
"Table Properties");
+ /**
+ * Guardrail warning about, ignoring or rejecting the usage of certain
keyspace properties.
+ */
+ public static final Values<String> keyspaceProperties =
+ new Values<>("keyspace_properties",
+ null,
+ state ->
CONFIG_PROVIDER.getOrCreate(state).getKeyspacePropertiesWarned(),
+ state ->
CONFIG_PROVIDER.getOrCreate(state).getKeyspacePropertiesIgnored(),
+ state ->
CONFIG_PROVIDER.getOrCreate(state).getKeyspacePropertiesDisallowed()
+ .stream()
+ .filter(prop -> !"replication".equals(prop))
Review Comment:
@aparnanaik0522 this is not what we have approved, seems like this went in
after the review. I really dont remember this being in before. I do not think
this is the correct way to do it. Every single time we execute guard method,
this "stream and filter" is going to be executed as side effect. This is not
necessary. Better way would be to filter out unsupported properties to disallow
in `setKeyspacePropertiesDisallowed` method and hence by proxy in
`validateKeyspaceProperties` which is used for other Values getter methods
(getKeyspacePropertiesWarned/Ignored ...).
Another issue is - why are we doing this for "disallowed" only? If you want
to completely bypass any logic related to `replication`, as of now, you might
_ignore it_ at guardrail level. So if somebody does
create keyspace abc with replication {dc1, rf: 5}
and you put "replication" among ignored values (which is currently
possible), a keyspace like this will be created, basically.
create keyspace abc;
which is actually invalid CQL.
Is this really what we want? I do not see why we would be preventing anybody
from specifying their replication factors for their keyspaces.
disallowed - means that a user can not specify query with "replication"
ignored - means that when a user specifies "replication" in a query, it will
be silently dropped as if it was never there
warned - means that a user can specify it and it will be taken into account
but a warning in CQLSH will be returned
We do not want to warn a user when "replication" is specified, right? Nor we
want to ignore "replication" parameter silently, nor we want to disallow usage
of "replication". Hence I think the proper way would be to filter it out
explicitly in validateKeyspaceProperties (and also inform a user, by logging,
that such a property is ignored altogether when using this guardrail).
cc @maedhroz
--
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]
