> Does this sound good? Sounds good to me
> On Jun 16, 2023, at 8:30 AM, Dan Jatnieks <jatni...@pobox.com> wrote: > > Hi all, > > Apologies for the late reply; I didn't mean to start a thread and then > disappear - it was unintended and I feel bad about that. > > I've been taking notes to summarize the discussion points and it matches what > Andres already listed, so I'm glad for that. And thank you Andres for doing > that - much appreciated! > > The plan Andres outlined also sounds good to me. I was not aware of @Replaces > before, and now that I learned it, I agree it should be used here. > > Converting the old threshold values by subtracting the system keyspace/tables > makes sense to keep the existing guardrail semantics - and including a > message about that will be a good step to reduce any confusion about how the, > possibly odd-looking, new value was determined. > > Dan > > > On Fri, Jun 16, 2023 at 9:58 AM Ekaterina Dimitrova <e.dimitr...@gmail.com > <mailto:e.dimitr...@gmail.com>> wrote: >> Hi all, >> I was following the discussion. What Andres just summarized sounds >> reasonable to me. Let’s just not forget to document also all this. >> Thank you >> Ekaterina >> >> On Fri, 16 Jun 2023 at 10:16, Andrés de la Peña <adelap...@apache.org >> <mailto:adelap...@apache.org>> wrote: >>> It seems we agree on removing the default value for the old thresholds, and >>> don't count system keyspaces/tables on the new ones. >>> >>> The old thresholds were on active duty for around ten months, and they have >>> been deprecated for around a year. They will have been deprecated for >>> longer by the time we release 5.0. If we want to keep them in perpetuity, I >>> guess the plan would be: >>> >>> - Remove the default value of the old thresholds in Config.java to make >>> them disabled by default. >>> - Remove the old thresholds from the default cassandra.yaml, although old >>> yamls can still have them. >>> - Use converters (@Replaces tag in Config.java) to read the old threshold >>> values (if present) and apply them to the new guardrails. >>> - During the conversion from the old thresholds to the new guardrails, >>> subtract the current number of system keyspace/tables from the old value. >>> For example, 150 tables in the old threshold translate to 103 tables in the >>> new guardrail, considering that there are 47 system tables. >>> >>> Does this sound good? >>> >>> On Wed, 14 Jun 2023 at 17:26, David Capwell <dcapw...@apple.com >>> <mailto:dcapw...@apple.com>> wrote: >>>>> That's problematic because the new thresholds we added in CASSANDRA-17147 >>>>> don't include system tables. Do you think we should change that? >>>> >>>> I wouldn’t change the semantics of the config as it’s already live. I >>>> guess where I am coming from is that logically we have to think about the >>>> system tables, so to your point, if we think 150 is too much and the >>>> system already exposes 50… then we should recommend no more than 100…. >>>> >>>>> I find it's better for usability to not count the system tables and just >>>>> say "It's recommended not to have more than 100 tables. This doesn't >>>>> include system tables.” >>>> >>>> >>>> I am fine with this framing… internally we think about 150 but publicly >>>> speak 100 (due to our 50 tables)... >>>> >>>> >>>>> On Jun 14, 2023, at 8:29 AM, Josh McKenzie <jmcken...@apache.org >>>>> <mailto:jmcken...@apache.org>> wrote: >>>>> >>>>>> In my opinion including system tables defeats that purpose because it >>>>>> forces users to know details about the system tables. >>>>> Perhaps having a unit test that caps our system tables at some value and >>>>> keeping the guardrail user-scope specific would be a better approach. I >>>>> see your point about leaking internal details to users, specifically on >>>>> things they can't control at this point. >>>>> >>>>> On Wed, Jun 14, 2023, at 8:19 AM, Andrés de la Peña wrote: >>>>>> > Default value I agree with you; features should be off by default! If >>>>>> > we remove the default then we disable the feature by default (which im >>>>>> > cool with) and for anyone who changed the config, they would keep >>>>>> > their behavior >>>>>> >>>>>> I'm glad we agree on at least removing the default value if we keep the >>>>>> deprecated properties. >>>>>> >>>>>> > With that, I kinda don’t agree that including system tables is a >>>>>> > mistake, as we add more we allow less for user tables before we start >>>>>> > to have issues…. >>>>>> >>>>>> That's problematic because the new thresholds we added in >>>>>> CASSANDRA-17147 don't include system tables. Do you think we should >>>>>> change that? >>>>>> >>>>>> I still think it's better not to include the system tables in the count. >>>>>> The thresholds on the number of keyspaces/tables/rows/columns/tombstones >>>>>> are just guidance since they cannot be exactly related to exact resource >>>>>> consumption. The main purpose of those thresholds is to prevent obvious >>>>>> antipatterns such as creating thousands of tables. A benefit of >>>>>> expressing the guardrails in terms of the number of schema entities, >>>>>> rather than counting the memory usage of those entities, is that they >>>>>> are easy to understand and reason about. In my opinion including system >>>>>> tables defeats that purpose because it forces users to know details >>>>>> about the system tables. The fact that those details change between >>>>>> versions doesn't help. Including system tables is not going to make the >>>>>> thresholds precise in terms of measuring memory consumption because that >>>>>> depends on other factors, such as the columns they store. >>>>>> >>>>>> Including system tables also imposes a minimum threshold value, like in >>>>>> 5.0 you cannot set a threshold value under 45 tables without triggering >>>>>> it with an empty db. For other thresholds, this can be more tricky. That >>>>>> would be the case of the guardrail on the number of columns in a >>>>>> partition, where you would need to know the size of the widest row in >>>>>> the system tables, which can change over time. >>>>>> >>>>>> I guess that if system tables were to be counted, a recommendation for >>>>>> the threshold would say something like "It's recommended not to have >>>>>> more than 150 tables. The system already includes 45 tables for internal >>>>>> usage, so you shouldn't create more than 105 user tables". I find it's >>>>>> better for usability to not count the system tables and just say "It's >>>>>> recommended not to have more than 100 tables. This doesn't include >>>>>> system tables." >>>>>> >>>>>> On Tue, 13 Jun 2023 at 23:51, Josh McKenzie <jmcken...@apache.org >>>>>> <mailto:jmcken...@apache.org>> wrote: >>>>>> >>>>>>> Warning that too many tables (including system) may have negative >>>>>>> behavior I think is fine >>>>>> This reminds me of the current situation with our tests where we just >>>>>> keep adding more and more without really considering the value of the >>>>>> current set and the costs of that body of work as it keeps growing. >>>>>> >>>>>> Having some kind of signal that we need to do some housekeeping with our >>>>>> system tables, or something in the feedback loop that helps us keep on >>>>>> top of this hygiene over time, seems like a clear benefit to me. >>>>>> >>>>>> On Tue, Jun 13, 2023, at 1:42 PM, David Capwell wrote: >>>>>>>> I think that the combined decision of using a default value and >>>>>>>> counting system tables was a mistake >>>>>>> >>>>>>> Default value I agree with you; features should be off by default! If >>>>>>> we remove the default then we disable the feature by default (which im >>>>>>> cool with) and for anyone who changed the config, they would keep their >>>>>>> behavior >>>>>>> >>>>>>> As for system tables… each table adds a cost to our bookkeeping, so >>>>>>> when we add new tables the cost grows and the memory per table >>>>>>> decreases, does it not? Warning that too many tables (including >>>>>>> system) may have negative behavior I think is fine, its only if we >>>>>>> start to fail is when things become a problem (upgrading to 5.0 can’t >>>>>>> happen due to too many tables added in the release?); think the feature >>>>>>> was warn only, so that should be fine. With that, I kinda don’t agree >>>>>>> that including system tables is a mistake, as we add more we allow less >>>>>>> for user tables before we start to have issues…. At the same time, if >>>>>>> we have improvements in newer versions that allows higher number of >>>>>>> tables, the user then has to update their configs (well, as long as we >>>>>>> don’t make things worse a smaller limit than needed is fine…) >>>>>>> >>>>>>>> we would need to know how many system keyspaces/tables were on the >>>>>>>> version we are upgrading from >>>>>>> >>>>>>> Do we? The logic was pulling from local schema, so to keep the same >>>>>>> behavior we would need to do the same; being version dependent would >>>>>>> actually break the semantics as far as I can tell. >>>>>>> >>>>>>>> On Jun 13, 2023, at 9:50 AM, Andrés de la Peña <adelap...@apache.org >>>>>>>> <mailto:adelap...@apache.org>> wrote: >>>>>>>> >>>>>>>> Indeed "keyspace_count_warn_threshold" and >>>>>>>> "table_count_warn_threshold" include system keyspaces and tables. >>>>>>>> Also, differently to the newer guardrails, they are enabled by >>>>>>>> default. >>>>>>>> >>>>>>>> I find that problematic because users need to know how many system >>>>>>>> keyspaces/tables there are to know if they need to set the threshold >>>>>>>> value. Moreover, if a new release adds some system tables, the >>>>>>>> threshold can start to be triggered without changing the number of >>>>>>>> user tables. That would force some users to update the threshold >>>>>>>> values during an upgrade. Even if they are using the defaults. That >>>>>>>> situation would happen again in any release adding new >>>>>>>> keyspaces/tables. I think adding new system tables is not that >>>>>>>> uncommon, and indeed 5.0 does it. >>>>>>>> >>>>>>>> I think that the combined decision of using a default value and >>>>>>>> counting system tables was a mistake. If that's the case, I don't know >>>>>>>> for how long we want to remain tied to that mistake. Especially when >>>>>>>> the old thresholds tend to create upgrade issues on their own. >>>>>>>> >>>>>>>> If we were going to use converters, we would need to know how many >>>>>>>> system keyspaces/tables were on the version we are upgrading from. I >>>>>>>> don't know if that information is available. Or perhaps we could >>>>>>>> assume that counting system keyspaces/tables was a bug, and just >>>>>>>> translate changing the meaning to not include them. >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> On Tue, 13 Jun 2023 at 16:51, David Capwell <dcapw...@apple.com >>>>>>>> <mailto:dcapw...@apple.com>> wrote: >>>>>>>> > Have we been dropping support entirely for old params or using the >>>>>>>> > @Replaces annotation into perpetuity? >>>>>>>> >>>>>>>> >>>>>>>> My understanding is that the goal is to keep things around in >>>>>>>> perpetuity unless it actively causes us harm… and with @Replaces, >>>>>>>> there tends to be no harm to keep around… >>>>>>>> >>>>>>>> Looking at >>>>>>>> https://github.com/apache/cassandra/commit/bae92ee139b411c94228f8fd5bb8befb4183ca9f >>>>>>>> we just marked them deprecated and created a brand new config that >>>>>>>> matched the old… which I feel was a bad idea…. Renaming configs are >>>>>>>> fine with @Replaces, but asking users to migrate with the idea of >>>>>>>> breaking them in the future is bad… >>>>>>>> >>>>>>>> The table_count_warn_threshold config is used at >>>>>>>> org.apache.cassandra.cql3.statements.schema.CreateTableStatement#clientWarnings >>>>>>>> The tables_warn_threshold config is used at >>>>>>>> org.apache.cassandra.cql3.statements.schema.CreateTableStatement#validate >>>>>>>> >>>>>>>> The only difference I see is that table_count_warn_threshold includes >>>>>>>> system tables where as tables_warn_threshold is only user tables… >>>>>>>> >>>>>>>> > I would like to propose removing the non-guardrail thresholds >>>>>>>> > 'keyspace_count_warn_threshold' and 'table_count_warn_threshold' >>>>>>>> > configuration settings on the trunk branch for the next major >>>>>>>> > release. >>>>>>>> >>>>>>>> Deprecate in 4.1 is way too new for me to accept that, and its low >>>>>>>> effort to keep; breaking users is always a bad idea and doing it when >>>>>>>> not needed is bad… >>>>>>>> >>>>>>>> Honestly, I don’t see why we couldn’t use @Replaces here to solve the >>>>>>>> semantic gap… table_count_warn_threshold includes the system tables, >>>>>>>> so we just need a Converter that takes w/e the value the user put in >>>>>>>> and subtracts the system tables… which then gives us the user tables >>>>>>>> (matching tables_warn_threshold) >>>>>>>> >>>>>>>> > On Jun 13, 2023, at 7:57 AM, Josh McKenzie <jmcken...@apache.org >>>>>>>> > <mailto:jmcken...@apache.org>> wrote: >>>>>>>> > >>>>>>>> >> have subsequently been deprecated since 4.1-alpha in >>>>>>>> >> CASSANDRA-17195 when they were replaced/migrated to guardrails as >>>>>>>> >> part of CEP-3 (Guardrails). >>>>>>>> > Have we been dropping support entirely for old params or using the >>>>>>>> > @Replaces annotation into perpetuity? >>>>>>>> > >>>>>>>> > I dislike the idea of operators having to remember to update things >>>>>>>> > between versions and being surprised when things change roughly >>>>>>>> > equally to us carrying along undocumented deprecated param name >>>>>>>> > mapping roughly equally. :) >>>>>>>> > >>>>>>>> > On Mon, Jun 12, 2023, at 5:56 PM, Dan Jatnieks wrote: >>>>>>>> >> Hello everyone, >>>>>>>> >> >>>>>>>> >> I would like to propose removing the non-guardrail thresholds >>>>>>>> >> 'keyspace_count_warn_threshold' and 'table_count_warn_threshold' >>>>>>>> >> configuration settings on the trunk branch for the next major >>>>>>>> >> release. >>>>>>>> >> >>>>>>>> >> These thresholds were first added with CASSANDRA-16309 in 4.0-beta4 >>>>>>>> >> and have subsequently been deprecated since 4.1-alpha in >>>>>>>> >> CASSANDRA-17195 when they were replaced/migrated to guardrails as >>>>>>>> >> part of CEP-3 (Guardrails). >>>>>>>> >> >>>>>>>> >> I'd appreciate any thoughts about this. I will open a ticket to get >>>>>>>> >> started if there is support for doing this. >>>>>>>> >> >>>>>>>> >> Reference: >>>>>>>> >> https://issues.apache.org/jira/browse/CASSANDRA-16309 >>>>>>>> >> https://issues.apache.org/jira/browse/CASSANDRA-17195 >>>>>>>> >> CEP-3: Guardrails >>>>>>>> >> https://cwiki.apache.org/confluence/display/CASSANDRA/CEP-3%3A+Guardrails >>>>>>>> >> >>>>>>>> >> >>>>>>>> >> Thanks, >>>>>>>> >> Dan Jatnieks >>>>
