> 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> 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> 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> 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 >>>
