[
https://issues.apache.org/jira/browse/CASSANDRA-18042?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17641897#comment-17641897
]
Andres de la Peña commented on CASSANDRA-18042:
-----------------------------------------------
The ordering with {{featue_enabled}} at the top makes sense to me. Probably not
only before the warning but before anything else. So, extending a bit the
example, it would look like:
{code:java}
sai_indexes_enabled: false
sai_indexes_warned: true
sai_indexes_write_buffer_size: 100MiB
sai_indexes_take_snapshots_enabled: false
sai_indexes_take_snapshots_warned: true
sai_indexes_take_snapshots_compressor: LZ4
{code}
As for the dependency relationship between {{featue_enabled}} and the warning
expressed by the {{_if_enabled_}} \{_}suffix. I think that dependency doesn't
happen only for the warning. In fact, all {{feature}}{_}{{{}*{}}} properties
also depend on {{{}featue_enabled{}}}, since they are all noop if the feature
is disabled. I think that dependency would be easy to understand if we put
{{featue_enabled}} and the top, and maybe reinforce the idea with comments on
the properties. Otherwise, if we were going to be terribly strict with the
application of the {{_if_enabled}} suffix to all dependant properties, the
config could be as confusing as:
{code:java}
sai_indexes_enabled: false
sai_indexes_warned_if_enabled: true
sai_indexes_write_buffer_size_if_enabled: 100MiB
sai_indexes_take_snapshots_if_enabled_enabled: false
sai_indexes_take_snapshots_if_enabled_warned_if_enabled: true
sai_indexes_take_snapshots_if_enabled_compressor_if_enabled: LZ4
{code}
I think we don't want that format.
Note also that there is also a similar dependency between the soft and hard
limits of the other guardrails. For example, triggering the hard limit of a
threshold will omit the warning. Or using a disallowed value will avoid the
warning on warned values. Here we are doing the same but with a boolean
threshold.
{quote}If you do not want to do that, I am not sure how to achieve that, maybe
having a guardrail which would accept more values?
{code:java}
sai_indexes: {enabled, warned, disabled}
{code}
{quote}
Not a bad idea for boolean guardrails such as {{{}EnableFlag{}}}, although
harder to achieve with not-boolean guardrails. However, we already have
multiple {{feature_enabled}} flags. Some of those are implemented as
guardrails, some aren't. We can't easily change them to a different format, so
I think it's desirable to keep {{feature_enabled}} for compatibility.
{quote}I think it's worth a little extra time here since we're establishing a
precedent (warn / fail threshold on feature flagged things)
{quote}
Indeed, a bad decision at naming conventions can stay with us for a long time.
{quote}I think we're kind of bikeshedding between a bunch of dissatisfying
options here as we try to bolt something on to a feature (guardrails as feature
flag) that was never intended to have the notion of "optionally warn if someone
takes the extra step to turn this feature on".
{quote}
Some of the current not-guardrail experimental feature flags also throw a
warning when the feature is enabled. Their behaviour is hardcoded and it's not
very consistent between features. If I'm right, we have warnings for SASI
indexes and MVs, but not for transient replication or dropping compact storage.
Adding the notion of the warning to the guardrail gives us a standarized way to
implement this kind of flags. This can be done without necessarily having a
yaml property for the warning, it can be hardcoded in the guardrail constructor
for each case. However, having the yaml property to enable/disable the warning
would allow users to get rid of the default warning if they think they know
what they are doing.
> Implement a guardrail for not having zero default ttl on tables with TWCS
> -------------------------------------------------------------------------
>
> Key: CASSANDRA-18042
> URL: https://issues.apache.org/jira/browse/CASSANDRA-18042
> Project: Cassandra
> Issue Type: Improvement
> Components: Feature/Guardrails, Legacy/Core
> Reporter: Stefan Miklosovic
> Assignee: Stefan Miklosovic
> Priority: Normal
> Fix For: 4.x
>
> Time Spent: 3h
> Remaining Estimate: 0h
>
> A user was surprised that his data have not started to expire after 90 days
> on his TWCS, he noticed that default_time_to_live on the table was set to 0
> (by accident from his side) and inserts were using TTL = 0 too.
> It is questionable why it it possible to create a table with TWCS and enable
> a user to specify default_time_to_live to be zero.
> On the other hand, I would argue that having default_time_to_live set to 0 on
> TWCS does not necessarily mean that such combination is illegal. It is about
> people just using that with advantage very often so tables are compacted away
> nicely. However, that does not have to mean that they could not use it with
> 0. But I yet have to see a use-case where TWCS was used and default ttl was
> set to 0 on purpose. Merely looking into Cassandra codebase, there are only
> cases when this parameter is not 0.
> There are three approaches:
> 1) just reject such statements (for CreateTable and AlterTable statements)
> where default_time_to_live = 0
> 2) Implement a guardrail for 1) so it can be enabled / disabled on demand
> 3) Leave possibility to set default_time_to_live to 0 on a table but make a
> guardrail for UpdateStatement so it might reject queries for tables with
> default_time_to_live is zero and for which its TTL (on that update statement)
> is set to 0 too.
> I would be careful about making the current configuration illegal because of
> backward compatibility. For that reason 2) makes the most sense to me.
> Maybe implementing 3) would make sense as well. There might be a table which
> has default ttl set to 0 as it expects a user to supply TTL every time.
> However, as it is not currently enforced anywhere, a client might still
> insert TTLs to be set to 0 even by accident.
> POC for 2) is here
> https://github.com/instaclustr/cassandra/commit/0b4dcc3d3deeffa393c02a3b80e27482007f9579
--
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
