[
https://issues.apache.org/jira/browse/CASSANDRA-18042?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17637349#comment-17637349
]
Stefan Miklosovic edited comment on CASSANDRA-18042 at 11/22/22 4:21 PM:
-------------------------------------------------------------------------
Thanks [~adelapena] for the review. I ve already incorporated your suggestions.
I do not mind to call it zero_default_ttl_on_twcs_enabled. With comments
included in cassandra.yaml it is pretty descriptive. However, I left MBean
method as it was, people invoking that does not have any documentation to look
into when called by tooling and it is better to be as much specific as possible.
PR reflects these changes.
So the only question is whether we should have warning flag too. For the
maximum flexibility we may include it. It is acceptable to have it if it is by
default enabled.
If we are going to have these properties:
{code:java}
zero_default_ttl_on_twcs_enabled: true
zero_default_ttl_on_twcs_warn: true
{code}
I think it would be better to call it like this:
{code:java}
zero_default_ttl_on_twcs_fail: false
zero_default_ttl_on_twcs_warn: true
{code}
We would then need to "revert" the logic so it would look like (notice negation
in the body of that if)
{code:java}
if (params.defaultTimeToLive == 0
&& !SchemaConstants.isSystemKeyspace(keyspaceName)
&&
TimeWindowCompactionStrategy.class.isAssignableFrom(params.compaction.klass()))
!Guardrails.zeroDefaultTTLOnTWCSEnabled.ensureEnabled(state);
{code}
This is a little bit hard to follow, what would be nice to have is to have
methods called "ensureDisabled" which is more natural to me so it would be like
{code:java}
if (params.defaultTimeToLive == 0
&& !SchemaConstants.isSystemKeyspace(keyspaceName)
&&
TimeWindowCompactionStrategy.class.isAssignableFrom(params.compaction.klass()))
Guardrails.zeroDefaultTTLOnTWCS.ensureDisabled(state);
{code}
what do you think?
EDIT:
looking at this more intensively, it is weird to be sure something is disabled
on a guardrail called "zeroDefaultTTLOnTWCS". It reads as if we are making sure
that zero ttl is NOT enabled, which is wrong as we want to proceed if it is
enabled.
was (Author: smiklosovic):
Thanks [~adelapena] for the review. I ve already incorporated your suggestions.
I do not mind to call it zero_default_ttl_on_twcs_enabled. With comments
included in cassandra.yaml it is pretty descriptive. However, I left MBean
method as it was, people invoking that does not have any documentation to look
into when called by tooling and it is better to be as much specific as
possible.
PR reflects these changes.
So the only question is whether we should have warning flag too. For the
maximum flexibility we may include it. It is acceptable to have it if it is by
default enabled.
If we are going to have these properties:
{code}
zero_default_ttl_on_twcs_enabled: true
zero_default_ttl_on_twcs_warn: true
{code}
I think it would be better to call it like this:
{code}
zero_default_ttl_on_twcs_fail: false
zero_default_ttl_on_twcs_warn: true
{code}
We would then need to "revert" the logic so it would look like (notice negation
in the body of that if)
{code}
if (params.defaultTimeToLive == 0
&& !SchemaConstants.isSystemKeyspace(keyspaceName)
&&
TimeWindowCompactionStrategy.class.isAssignableFrom(params.compaction.klass()))
!Guardrails.zeroDefaultTTLOnTWCSEnabled.ensureEnabled(state);
{code}
This is a little bit hard to follow, what would be nice to have is to have
methods called "ensureDisabled" which is more natural to me so it would be like
{code}
if (params.defaultTimeToLive == 0
&& !SchemaConstants.isSystemKeyspace(keyspaceName)
&&
TimeWindowCompactionStrategy.class.isAssignableFrom(params.compaction.klass()))
Guardrails.zeroDefaultTTLOnTWCS.ensureDisabled(state);
{code}
what do you think?
> 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: 0.5h
> 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
