CASSANDRA-19677: Adds new size Guardrails #3355
Conversation
CASSANDRA-19677
conf/cassandra.yaml
Outdated
| # Guardrail to warn or fail when writing ascii column values larger than threshold. | ||
| # This guardrail is only applied to the values of regular columns because both the serialized partitions keys and the | ||
| # values of the components of the clustering key already have a fixed, relatively small size limit of 65535 bytes, which | ||
| # is probably lesser than the thresholds defined here. | ||
| # The two thresholds default to null to disable. |
There was a problem hiding this comment.
Can you add the clarification when both column_value_size_warn_threshold and column_ascii_value_size_warn_threshold are enabled?
For example, column_value_size_warn_threshold=100B and column_ascii_value_size_warn_threshold=200B. If the incoming write has the ascii field size of 123B, should it warn or not?
| if (column.type instanceof AsciiType) // Ascii size specific guardrail | ||
| { | ||
| Guardrails.columnAsciiValueSize.guard(value.remaining(), column.name.toString(), false, clientState); | ||
| } else if (column.type instanceof BytesType) // Blob size specific guardrail |
There was a problem hiding this comment.
move to new line. There are a few other places that the else if is placed wrongly. Please correct them
| } else if (column.type instanceof UTF8Type) // text and varchar size specific guardrails | ||
| { | ||
| Guardrails.columnTextValueSize.guard(value.remaining(), column.name.toString(), false, clientState); | ||
| Guardrails.columnVarcharValueSize.guard(value.remaining(), column.name.toString(), false, clientState); | ||
| } |
There was a problem hiding this comment.
It is confusing (sometimes surprising) to end-users to have 2 guardrails to check on the same value. Since text and varchar are identical internally, can you combine those 2 guardrails?
| Guardrails.columnValueSize.guard(value.remaining(), column.name.toString(), false, clientState); | ||
|
|
||
| if (column.type instanceof AsciiType) // Ascii size specific guardrail |
There was a problem hiding this comment.
nit: prefer comparing the enum type, instead of instanceof, e.g.
| if (column.type instanceof AsciiType) // Ascii size specific guardrail | |
| if (column.type.asCQL3Type() == CQL3Type.Native.ASCII) |
Style fixes and merge text and varchar guardrail.
| Guardrails.columnValueSize.guard(value.remaining(), column.name.toString(), false, clientState); | ||
|
|
||
| if (column.type.asCQL3Type() == CQL3Type.Native.ASCII) // Ascii size specific guardrail |
There was a problem hiding this comment.
Why not a switch statement here? if not, no need for the else's with the nested ifs, it makes it more verbose -- use else if instead. Also think moving this to a validateSize function (or something similarly named) would make it more clear
CASSANDRA-19677: Adds new size Guardrails
This commit adds these column size guardrails:
And these collection size guardrails:
patch by Bernardo Botella Corbi; reviewed by for CASSANDRA-19677