ekaterinadimitrova2 commented on code in PR #1758: URL: https://github.com/apache/cassandra/pull/1758#discussion_r931534790
########## conf/cassandra.yaml: ########## @@ -992,6 +992,8 @@ compaction_throughput: 64MiB/s # are completely written, and used in place of the prior sstables for # any range that has been written. This helps to smoothly transfer reads # between the sstables, reducing page cache churn and keeping hot rows hot +# Set sstable_preemptive_open_interval to null for disabled which is equivalent to the sstable_preemptive_open_interval_in_mb +# being negative Review Comment: I already fixed it, but if we want a flag we can do that of course. The thing is the usage of null was accepted and documented long time ago, if we do it here differently, does this mean we need to change it in all other places we did use null? In that case this would fix the issue with null and the VT as it seems that check is important for a work done in CASSANDRA-17166 as if I remove it VT throws currently null pointer exception. I have to check what @dcapwell did in that patch as I was not really working on that ticket. (CASSANDRA-17166) Looking back at the `Converters`, there are 4 other properties where we depend on `null`: `credentials_update_interval, permissions_update_interval, roles_update_interval` - those are actually interesting in the context of this ticket as JMX uses the getters and returns their `*_validity` equivalent if they are null but VT shows null/-1... I will produce a patch for them separately. This is also a 4.0 bug The 4-th one dependent on the null value (which I just fixed to be able to start with null, seems I didn't fully fix it the other day...) is `index_summary_resize_interval`. I will fix it after we agree which direction we prefer - flags or the null value. Now the only uncomfortable situation I see is that people will have to maintain two properties for one old one and if we continue adding feature flags and not removing properties, our config becomes too big and scary. `Does the new config framework have the capability to drive to settings from the deprecated sstable_preemptive_open_interval_in_mb?` If you set the old one with `sstable_preemptive_open_interval_in_mb=-1` in cassandra.yaml, it loads internally through the `@Replaces` annotation and the `Converters` class into `sstable_preemptive_open_interval=null`. That is how the backward compatibility works on the parsing level for cassandra.yaml. I hope I didn't misunderstood your question. ########## conf/cassandra.yaml: ########## @@ -992,6 +992,8 @@ compaction_throughput: 64MiB/s # are completely written, and used in place of the prior sstables for # any range that has been written. This helps to smoothly transfer reads # between the sstables, reducing page cache churn and keeping hot rows hot +# Set sstable_preemptive_open_interval to null for disabled which is equivalent to the sstable_preemptive_open_interval_in_mb +# being negative Review Comment: I already fixed it, but if we want a flag we can do that of course. The thing is the usage of null was accepted and documented long time ago, if we do it here differently, does this mean we need to change it in all other places we did use null? In that case this would fix the issue with null and the VT as it seems that check is important for a work done in CASSANDRA-17166 as if I remove it VT throws currently null pointer exception. I have to check what @dcapwell did in that patch as I was not really working on that ticket. (CASSANDRA-17166) Looking back at the `Converters`, there are 4 other properties where we depend on `null`: `credentials_update_interval, permissions_update_interval, roles_update_interval` - those are actually interesting in the context of this ticket as JMX uses the getters and returns their `*_validity` equivalent if they are null but VT shows null/-1... I will produce a patch for them separately. This is also a 4.0 bug but there we use -1, not null The 4-th one dependent on the null value (which I just fixed to be able to start with null, seems I didn't fully fix it the other day...) is `index_summary_resize_interval`. I will fix it after we agree which direction we prefer - flags or the null value. Now the only uncomfortable situation I see is that people will have to maintain two properties for one old one and if we continue adding feature flags and not removing properties, our config becomes too big and scary. `Does the new config framework have the capability to drive to settings from the deprecated sstable_preemptive_open_interval_in_mb?` If you set the old one with `sstable_preemptive_open_interval_in_mb=-1` in cassandra.yaml, it loads internally through the `@Replaces` annotation and the `Converters` class into `sstable_preemptive_open_interval=null`. That is how the backward compatibility works on the parsing level for cassandra.yaml. I hope I didn't misunderstood your question. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
