You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "bbotella (via GitHub)" <gi...@apache.org> on 2024/06/07 16:59:09 UTC
[PR] CASSANDRA-19677: Adds new size Guardrails [cassandra]
bbotella opened a new pull request, #3355:
URL: https://github.com/apache/cassandra/pull/3355
CASSANDRA-19677: Adds new size Guardrails
This commit adds these column size guardrails:
```
column_ascii_value_size_warn_threshold
column_ascii_value_size_fail_threshold
column_blob_value_size_warn_threshold
column_blob_value_size_fail_threshold
column_text_value_size_warn_threshold
column_text_value_size_fail_threshold
column_varchar_value_size_warn_threshold
column_varchar_value_size_fail_threshold
```
And these collection size guardrails:
```
collection_map_size_warn_threshold
collection_map_size_fail_threshold
collection_set_size_warn_threshold
collection_set_size_fail_threshold
collection_list_size_warn_threshold
collection_list_size_fail_threshold
```
patch by Bernardo Botella Corbi; reviewed by <Reviewers> for CASSANDRA-19677
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-19677: Adds new size Guardrails [cassandra]
Posted by "pmcfadin (via GitHub)" <gi...@apache.org>.
pmcfadin commented on PR #3355:
URL: https://github.com/apache/cassandra/pull/3355#issuecomment-4227138263
Closing as [CASSANDRA-19677](https://issues.apache.org/jira/browse/CASSANDRA-19677) has been resolved as Fixed — if this PR contains changes not captured in the merged fix, please reopen.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-19677: Adds new size Guardrails [cassandra]
Posted by "pmcfadin (via GitHub)" <gi...@apache.org>.
pmcfadin closed pull request #3355: CASSANDRA-19677: Adds new size Guardrails
URL: https://github.com/apache/cassandra/pull/3355
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-19677: Adds new size Guardrails [cassandra]
Posted by "yifan-c (via GitHub)" <gi...@apache.org>.
yifan-c commented on code in PR #3355:
URL: https://github.com/apache/cassandra/pull/3355#discussion_r1949628834
##########
src/java/org/apache/cassandra/cql3/terms/Lists.java:
##########
@@ -414,12 +414,14 @@ static void doAppend(Term.Terminal value, ColumnMetadata column, UpdateParameter
dataSize += cell.dataSize();
}
Guardrails.collectionSize.guard(dataSize, column.name.toString(), false, params.clientState);
+ Guardrails.collectionListSize.guard(dataSize, column.name.toString(), false, params.clientState);
Review Comment:
Let's go a bit deeper and compare the difference. (Yes. They have difference behavior).
The curried function (my suggestion) defines the evaluation priority and check once. That is, as long as collectionListSize is defined, it takes the precedence, regardless of their threshold values. It makes sense, since this patch brings a set of more finer-grained guardrails regarding collection sizes.
The handling in this patch, check the size _twice_, if they are both defined/enabled. In this case, it is the lowest threshold value that enforce the guardrail. The value could come from `collectionListSize` or `collectionSize`. Imagining from users' perspective, if the size guardrail on List is defined, it could cause confusion if a list size that satisfies the list size guardrail fails (due to the lower threshold from collection size).
In the end, it needs documentation to elaborate the behaviors of the overlapping guardrails.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-19677: Adds new size Guardrails [cassandra]
Posted by "yifan-c (via GitHub)" <gi...@apache.org>.
yifan-c commented on code in PR #3355:
URL: https://github.com/apache/cassandra/pull/3355#discussion_r1950084562
##########
conf/cassandra.yaml:
##########
@@ -2365,8 +2363,8 @@ drop_compact_storage_enabled: false
# of non-frozen collections there could be unaccounted parts of the collection on the sstables. This is done this way to
# prevent read-before-write. The guardrail is also checked at sstable write time to detect large non-frozen collections,
# although in that case exceeding the fail threshold will only log an error message, without interrupting the operation.
-# Also, this guardrail can be used with the generic collection_size_fail_threshold. The lowest of the two guardrail will
-# be the one enforced.
+# When collection_map_size_warn_threshold is defined, it takes the precedence over generic collection_size_fail_threshold
+# guardrail.
Review Comment:
This still seems wrong to me.
```suggestion
# When collection_map_size_warn_threshold and/or collection_map_size_fail_threshold are defined, they take the precedence
# over the cooresponding collection_size_warn_threshold and/or collection_size_fail_threshold.
```
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-19677: Adds new size Guardrails [cassandra]
Posted by "yifan-c (via GitHub)" <gi...@apache.org>.
yifan-c commented on code in PR #3355:
URL: https://github.com/apache/cassandra/pull/3355#discussion_r1948307988
##########
src/java/org/apache/cassandra/cql3/terms/Lists.java:
##########
@@ -414,12 +414,14 @@ static void doAppend(Term.Terminal value, ColumnMetadata column, UpdateParameter
dataSize += cell.dataSize();
}
Guardrails.collectionSize.guard(dataSize, column.name.toString(), false, params.clientState);
+ Guardrails.collectionListSize.guard(dataSize, column.name.toString(), false, params.clientState);
Review Comment:
I am wondering if you can sort of extending the `collectionListSize` from `collectionSize` and only call `collectionListSize` here, to avoid the "repetition". Similarly, it can be applied for map and set.
Basically, something like this. Note the `warnThreshold` and `failThreshold` functions.
```java
public static final MaxThreshold collectionListSize =
new MaxThreshold("collection_list_size",
null,
state -> {
Long listSizeWarnThreshold = sizeToBytes(CONFIG_PROVIDER.getOrCreate(state).getCollectionListSizeWarnThreshold());
return Objects.requireNonNullElseGet(listSizeWarnThreshold, () -> collectionSize.warnValue(state));
},
state -> {
Long listSizeWarnThreshold = sizeToBytes(CONFIG_PROVIDER.getOrCreate(state).getCollectionListSizeFailThreshold());
return Objects.requireNonNullElseGet(listSizeWarnThreshold, () -> collectionSize.failValue(state));
},
(isWarning, what, value, threshold) ->
format("Detected collection %s of size %s, this exceeds the %s threshold of %s.",
what, value, isWarning ? "warning" : "failure", threshold));
```
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-19677: Adds new size Guardrails [cassandra]
Posted by "bbotella (via GitHub)" <gi...@apache.org>.
bbotella commented on code in PR #3355:
URL: https://github.com/apache/cassandra/pull/3355#discussion_r1949175421
##########
src/java/org/apache/cassandra/cql3/terms/Lists.java:
##########
@@ -414,12 +414,14 @@ static void doAppend(Term.Terminal value, ColumnMetadata column, UpdateParameter
dataSize += cell.dataSize();
}
Guardrails.collectionSize.guard(dataSize, column.name.toString(), false, params.clientState);
+ Guardrails.collectionListSize.guard(dataSize, column.name.toString(), false, params.clientState);
Review Comment:
I hear you, and it makes sense. But I kind of like the verbosity of having the two separate guardrails explicitly called. I think from a readability standpoint, it makes it easier to see "both" guardrails being checked in order, instead of hiding the check of the former as part of the check of the latter. Again, on the fence on this one, so, if you feel this is the right way to go, I can definitely change it.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-19677: Adds new size Guardrails [cassandra]
Posted by "jrwest (via GitHub)" <gi...@apache.org>.
jrwest commented on code in PR #3355:
URL: https://github.com/apache/cassandra/pull/3355#discussion_r1669421736
##########
src/java/org/apache/cassandra/cql3/UpdateParameters.java:
##########
@@ -163,8 +163,28 @@ public Cell<?> addCell(ColumnMetadata column, ByteBuffer value) throws InvalidRe
public Cell<?> addCell(ColumnMetadata column, CellPath path, ByteBuffer value) throws InvalidRequestException
{
+ // General column value size
Guardrails.columnValueSize.guard(value.remaining(), column.name.toString(), false, clientState);
+ if (column.type.asCQL3Type() == CQL3Type.Native.ASCII) // Ascii size specific guardrail
Review 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
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org