-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CASSANDRA-19677: Adds new size Guardrails #3355
base: trunk
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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