[
https://issues.apache.org/jira/browse/CASSANDRA-19552?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18004181#comment-18004181
]
Stefan Miklosovic edited comment on CASSANDRA-19552 at 7/9/25 4:16 PM:
-----------------------------------------------------------------------
Paulo suggested that putting this into 5.0 might be acceptable and I tend to
agree with him (I was biased towards CQL but I see there is quite a push for
nodetool as well so ... ) I am not sure about 4.1 but 5.0 seems to be just OK.
I took a look at original patch of Yuqi and reviewed. I have addressed
everything but 10) in [https://github.com/apache/cassandra/pull/4237]
1)
I am suprised that getguardrailsconfig is by default returning just this:
{code:java}
$ ./bin/nodetool getguardrailsconfig
Guardrails Configuration:
SaiFrozenTermSizeFailThreshold: 8KiB
SaiFrozenTermSizeWarnThreshold: 1KiB
SaiSSTableIndexesPerQueryWarnThreshold: 32
SaiStringTermSizeFailThreshold: 8KiB
SaiStringTermSizeWarnThreshold: 1KiB
SaiVectorTermSizeFailThreshold: 32KiB
SaiVectorTermSizeWarnThreshold: 16KiB
{code}
and I need to add --all to show it all. I should just get all of them without
any additional flags. This is misleading as it seems like these are all
guardrails there are. The reason these are returned only is that they are not
"null" / -1. I would just get rid of --all completely. I want to see all of
them even with default values. There is so many guardrails that seeing them all
just helps me to figure out how it is actually called.
2)
nodetool setguardrailsconfig (without any other arguments) fails on this:
{code:java}
./bin/nodetool setguardrailsconfig
error: Index 0 out of bounds for length 0
-- StackTrace --
java.lang.IndexOutOfBoundsException: Index 0 out of bounds for length 0
at
java.base/jdk.internal.util.Preconditions.outOfBounds(Preconditions.java:64)
at
java.base/jdk.internal.util.Preconditions.outOfBoundsCheckIndex(Preconditions.java:70)
at
java.base/jdk.internal.util.Preconditions.checkIndex(Preconditions.java:248)
at java.base/java.util.Objects.checkIndex(Objects.java:372)
at java.base/java.util.ArrayList.get(ArrayList.java:459)
at
org.apache.cassandra.tools.nodetool.SetGuardrailsConfig.execute(SetGuardrailsConfig.java:88)
at
org.apache.cassandra.tools.NodeTool$NodeToolCmd.runInternal(NodeTool.java:386)
at
org.apache.cassandra.tools.NodeTool$NodeToolCmd.run(NodeTool.java:371)
at org.apache.cassandra.tools.NodeTool.execute(NodeTool.java:270)
at org.apache.cassandra.tools.NodeTool.main(NodeTool.java:85)
{code}
3)
getguardrailsconfig returns method names without "get" but setguardrailsconfig,
when listed, returns them with "set". I think that in both cases we should do
it without "get/set" and for set command, "set" would be automatically appended
internally.
4)
There is no check on values used for e.g. booleans. This passes now ("a" is
converted to "false").
./bin/nodetool setguardrailsconfig ZeroTTLOnTWCSWarned a
We should be more explicit that enabled values are "true" / "false" and we
should harden parsing of values. (integers, longs, booleans ...)
5)
getguardrailsconfig returns
{code:java}
TablePropertiesWarned []
TablePropertiesWarnedCSV null{code}
There are two mbean methods for this type of setter. On server side it
translates to:
{code:java}
void setTablePropertiesDisallowed(Set<String> properties);
void setTablePropertiesDisallowedCSV(String properties); {code}
The current solution will error out when we use 'setTablePropertiesDisallowed'
setter (without CSV) on a string with values separated by comma. There is no
real reason why we would error out. We can just parse this string and create a
set for that which would be passed to corresponding setter method accepting
sets.
6) We should use TableBuilder for constructing the output.
7) For trunk branch, it is also possible to call MBean method accepting
Map<String, Object>. This was introduced by CEP-24 (Password validator). I am
not completely sure how to visualise this into nodetool (complex values as maps
to string). For trunk patch and for now we might just omit these setters /
getters completely.
8)
we should change CamelCase with snake_case when it comes to names of guardrails
shown in the output. snake_case is what is used in the configuration in
cassandra.yaml. It should be very easy to make this translation.
9)
We should have also this form of getting individual setting:
{code:java}
$ ./bin/nodetool getguardrailsconfig -n vector_type_enabled
true {code}
10)
What I would like to see is to be a little bit more explicit about the type of
guardrail we use for setting its value. For now, the usage for each type is
directly in the description of the tool which is quite uncommon. We might have
flags like:
{code:java}
--flag SomeGuardRail true
--threshold SomeGuardRail 10 20
--property SomeGuardRail a,b,c {code}
This is way more intentional about the type of guardrail we want to set and we
can have nice description of expected arguments for each case.
was (Author: smiklosovic):
Paulo suggested that putting this into 5.0 might be acceptable and I tend to
agree with him (I was biased towards CQL but I see there is quite a push for
nodetool as well so ... ) I am not sure about 4.1 but 5.0 seems to be just OK.
I took a look at original patch of Yuqi and reviewed. I have addressed
everything but 9) in https://github.com/apache/cassandra/pull/4237
1)
I am suprised that getguardrailsconfig is by default returning just this:
{code:java}
$ ./bin/nodetool getguardrailsconfig
Guardrails Configuration:
SaiFrozenTermSizeFailThreshold: 8KiB
SaiFrozenTermSizeWarnThreshold: 1KiB
SaiSSTableIndexesPerQueryWarnThreshold: 32
SaiStringTermSizeFailThreshold: 8KiB
SaiStringTermSizeWarnThreshold: 1KiB
SaiVectorTermSizeFailThreshold: 32KiB
SaiVectorTermSizeWarnThreshold: 16KiB
{code}
and I need to add --all to show it all. I should just get all of them without
any additional flags. This is misleading as it seems like these are all
guardrails there are. The reason these are returned only is that they are not
"null" / -1. I would just get rid of --all completely. I want to see all of
them even with default values. There is so many guardrails that seeing them all
just helps me to figure out how it is actually called.
2)
nodetool setguardrailsconfig (without any other arguments) fails on this:
{code:java}
./bin/nodetool setguardrailsconfig
error: Index 0 out of bounds for length 0
-- StackTrace --
java.lang.IndexOutOfBoundsException: Index 0 out of bounds for length 0
at
java.base/jdk.internal.util.Preconditions.outOfBounds(Preconditions.java:64)
at
java.base/jdk.internal.util.Preconditions.outOfBoundsCheckIndex(Preconditions.java:70)
at
java.base/jdk.internal.util.Preconditions.checkIndex(Preconditions.java:248)
at java.base/java.util.Objects.checkIndex(Objects.java:372)
at java.base/java.util.ArrayList.get(ArrayList.java:459)
at
org.apache.cassandra.tools.nodetool.SetGuardrailsConfig.execute(SetGuardrailsConfig.java:88)
at
org.apache.cassandra.tools.NodeTool$NodeToolCmd.runInternal(NodeTool.java:386)
at
org.apache.cassandra.tools.NodeTool$NodeToolCmd.run(NodeTool.java:371)
at org.apache.cassandra.tools.NodeTool.execute(NodeTool.java:270)
at org.apache.cassandra.tools.NodeTool.main(NodeTool.java:85)
{code}
3)
getguardrailsconfig returns method names without "get" but setguardrailsconfig,
when listed, returns them with "set". I think that in both cases we should do
it without "get/set" and for set command, "set" would be automatically appended
internally.
4)
There is no check on values used for e.g. booleans. This passes now ("a" is
converted to "false").
./bin/nodetool setguardrailsconfig ZeroTTLOnTWCSWarned a
We should be more explicit that enabled values are "true" / "false" and we
should harden parsing of values. (integers, longs, booleans ...)
5)
getguardrailsconfig returns
{code:java}
TablePropertiesWarned []
TablePropertiesWarnedCSV null{code}
There are two mbean methods for this type of setter. On server side it
translates to:
{code:java}
void setTablePropertiesDisallowed(Set<String> properties);
void setTablePropertiesDisallowedCSV(String properties); {code}
The current solution will error out when we use 'setTablePropertiesDisallowed'
setter (without CSV) on a string with values separated by comma. There is no
real reason why we would error out. We can just parse this string and create a
set for that which would be passed to corresponding setter method accepting
sets.
6) We should use TableBuilder for constructing the output.
7) For trunk branch, it is also possible to call MBean method accepting
Map<String, Object>. This was introduced by CEP-24 (Password validator). I am
not completely sure how to visualise this into nodetool (complex values as maps
to string). For trunk patch and for now we might just omit these setters /
getters completely.
8)
we should change CamelCase with snake_case when it comes to names of guardrails
shown in the output. snake_case is what is used in the configuration in
cassandra.yaml. It should be very easy to make this translation.
9)
What I would like to see is to be a little bit more explicit about the type of
guardrail we use for setting its value. For now, the usage for each type is
directly in the description of the tool which is quite uncommon. We might have
flags like:
{code:java}
--flag SomeGuardRail true
--threshold SomeGuardRail 10 20
--property SomeGuardRail a,b,c {code}
This is way more intentional about the type of guardrail we want to set and we
can have nice description of expected arguments for each case.
> Nodetool to get/set guardrails configurations
> ---------------------------------------------
>
> Key: CASSANDRA-19552
> URL: https://issues.apache.org/jira/browse/CASSANDRA-19552
> Project: Apache Cassandra
> Issue Type: Improvement
> Components: Feature/Guardrails
> Reporter: Yuqi Yan
> Assignee: Yuqi Yan
> Priority: Normal
> Fix For: 4.1.x
>
> Time Spent: 0.5h
> Remaining Estimate: 0h
>
> Currently guardrails are only configurable through JMX / cassandra.yaml
> This provides a nodetool command to interact with all the getters/setters for
> guardrails.
>
> 4.1 PR: [https://github.com/apache/cassandra/pull/3243]
> trunk PR: [https://github.com/apache/cassandra/pull/3244]
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
