17737 4.1 sstable preemptive open interval in mb #1758

ekaterinadimitrova2 · 2022-07-27T13:48:59Z

No description provided.

…compatibility

jonmeredith

Checking the virtual tables, I used the deprecated sstable_preemptive_open_interval_in_mb: -1 until I understand how to set the newer version. The backward compatibility doesn't handle converting back to sstable_preemptive_open_interval_in_mb and reports as null. I'm happy to live with it if it isn't an easy fix.

Connected to Test Cluster at 127.0.0.1:9042
[cqlsh 6.1.0 | Cassandra 4.1-alpha2-SNAPSHOT | CQL spec 3.4.6 | Native protocol v5]
Use HELP for help.
cqlsh> SELECT * FROM system_views.settings WHERE name = 'sstable_preemptive_open_interval';

 name                             | value
----------------------------------+-------
 sstable_preemptive_open_interval |  null

(1 rows)
cqlsh> SELECT * FROM system_views.settings WHERE name = 'sstable_preemptive_open_interval_in_mb';

 name                                   | value
----------------------------------------+-------
 sstable_preemptive_open_interval_in_mb |  null

Then tried setting to 99 using JMX which worked fine.

(1 rows)
cqlsh> SELECT * FROM system_views.settings WHERE name = 'sstable_preemptive_open_interval_in_mb';

 name                                   | value
----------------------------------------+-------
 sstable_preemptive_open_interval_in_mb |    99

(1 rows)
cqlsh> SELECT * FROM system_views.settings WHERE name = 'sstable_preemptive_open_interval';

 name                             | value
----------------------------------+-------
 sstable_preemptive_open_interval | 99MiB

(1 rows)

jonmeredith · 2022-07-27T17:21:27Z

src/java/org/apache/cassandra/config/DatabaseDescriptor.java

    public static void setSSTablePreemptiveOpenIntervalInMiB(int mib)
    {
-        conf.sstable_preemptive_open_interval = new DataStorageSpec.IntMebibytesBound(mib);
+        if (mib == -1)


Suggested change

if (mib == -1)

if (mib < 0)

jonmeredith · 2022-07-27T17:24:28Z

conf/cassandra.yaml

+# Set sstable_preemptive_open_interval to null for disabled which is equivalent to the sstable_preemptive_open_interval_in_mb
+# being negative


Comment is a lot longer than the rest of the block, would you mind wrapping it early. I'm guessing we still refer to deprecated parameters in other comments and we can remove them if the deprecated settings are actually removed.

How do you set to null? I tried

sstable_preemptive_open_interval: null

but got this on startup.

Exception (org.apache.cassandra.exceptions.ConfigurationException) encountered during startup: Invalid yaml. Those properties [sstable_preemptive_open_interval] are not valid ERROR [main] 2022-07-27 11:34:46,521 CassandraDaemon.java:896 - Exception encountered during startup: Invalid yaml. Those properties [sstable_preemptive_open_interval] are not valid Invalid yaml. Those properties [sstable_preemptive_open_interval] are not valid

then
sstable_preemptive_open_interval:

and got

ERROR [main] 2022-07-27 11:37:15,213 CassandraDaemon.java:896 - Exception encountered during startup: Invalid yaml. Those properties [sstable_preemptive_open_interval] are not valid Invalid yaml. Those properties [sstable_preemptive_open_interval] are not valid``` and of course `-1` still fails as expected.

Good catch, null is valid only if it is default value in the Config class which is not the case here :(
I should have seen that. I will have to think about how to workaround it and add a startup test as now I double checked and it seems I made the same mistake yesterday with one more property so now I have to fix both of them. Thank you for catching it!

About the -1, it starts for me with sstable_preemptive_open_interval_in_mb = -1, then we see in the logs the converter loaded it as sstable_preemptive_open_interval = null; But I definitely null doesn't work.

Thinking out loud.... we can do probably a mix of what you suggested initially and this fix for this special case. We can add a flag parameter to enable/disable which will be used with the new property if we want to disable it. If someone uses the old one we should prohibit setting the new flag then in parallel on startup if the old one was found in the yaml, WDYT?

I might be able to workaround that need of additional flag actually and special case those two properties so that they do not error out when set to null.

The more I think, the more I am convinced enable/disable flags should work for features and not individual properties

I hope I didn't misunderstood your question.

I was asking if sstable_preemptive_open_interval_in_mb could set a new sstable_preemptive_open_enabled entry as well as sstable_preemptive_open_interval.

If you have a fix for setting null and don't need to create an enabled flag I'm fine with that. I thought you were saying you might need to create on to resolve this problem.

I was asking if sstable_preemptive_open_interval_in_mb could set a new sstable_preemptive_open_enabled entry as well as sstable_preemptive_open_interval.
No, currently we map one or more old names to only one new property.

If you have a fix for setting null and don't need to create an enabled flag I'm fine with that. I thought you were saying you might need to create on to resolve this problem.
I created a quick one by special casing this particular property to be able to set it to null even if the default value is not null. So I will need to figure out then only the VT Converters

...and more tests to prevent us from future regressions of course :-)

Oh, seems it was me not that ticket that broke it (that one just changes to return null and not 0)... I already have a fix, adding more tests, will push a patch later tonight

ekaterinadimitrova2 · 2022-07-27T18:52:57Z

About the VT, the conversion happens in the last method in the converter which was not handling the special case, I will fix it in a moment.
The reverseConverter in Converter is what handles this, it should be in this case o -> o == null ? -1 : o.toMebibytes()),

ekaterinadimitrova2 · 2022-07-27T19:19:36Z

Ha...about the VT, it seems that someone added null check in the unconvert() method and broke the special cases we have in the converter... I will see to fix it and add more tests to prevent that from happening in the future

…ue for disabled Fix Settings Virtual Table backward compatibility for properties that would be assigned null now instead of -1 or negatives

ekaterinadimitrova2 · 2022-07-28T01:31:17Z

@jonmeredith I just pushed review commits to address your feedback and follow up discussion plus improved the testing. The only current issue I see is with the fromMap in the configuration loader which does not accept currently null values for properties that are not null by default in the Config class. I will try to figure it out tomorrow but at least we have a workaround and I wanted to show you what I came up with so far. Currently that method is also only used by the in-jvm test framework.
The Converter was broken when we introduced null post CASSANDRA-15234, I will add to the new tests for the rest of the properties and converters in a separate commit tomorrow. I didn't want to overwhelm this one. Only fixed the issue with index_summary_resize_interval=null which was the same as the one with sstable_preemptive_open_interval=null on startup

adelapena · 2022-07-28T12:46:35Z

.build/build-rat.xml

                 <exclude NAME="**/doc/antora.yml"/>
                 <exclude name="**/test/conf/cassandra.yaml"/>
                 <exclude name="**/test/conf/cassandra-old.yaml"/>
+                 <exclude name="**/cassandra-converters-special-cases-old-names.yaml"/>


I think this should be <exclude name="**/test/conf/cassandra-converters-special-cases-old-names.yaml"/>

Good point, it still works this way (tested locally) but it is inconsistent, I will change it, thanks

adelapena · 2022-07-28T12:47:36Z

NEWS.txt


 Upgrading
 ---------
+    - `sstable_preemptive_open_interval_in_mb` being negatve for disabled is equivalent to `sstable_preemptive_open_interval`


Suggested change

- `sstable_preemptive_open_interval_in_mb` being negatve for disabled is equivalent to `sstable_preemptive_open_interval`

- `sstable_preemptive_open_interval_in_mb` being negative for disabled is equivalent to `sstable_preemptive_open_interval`

adelapena · 2022-07-28T12:49:19Z

test/conf/cassandra-converters-special-cases-old-names.yaml

@@ -0,0 +1,3 @@
+sstable_preemptive_open_interval_in_mb: -1


Maybe we could add a brief comment at the beginning of this file indicating what it's used for.

adelapena · 2022-07-28T12:49:28Z

test/conf/cassandra-converters-special-cases.yaml

@@ -0,0 +1,3 @@
+sstable_preemptive_open_interval:


Maybe we could add a brief comment at the beginning of this file indicating what it's used for.

adelapena · 2022-07-28T14:25:42Z

src/java/org/apache/cassandra/config/Config.java


-    @Replaces(oldName = "sstable_preemptive_open_interval_in_mb", converter = Converters.MEBIBYTES_DATA_STORAGE_INT, deprecated = true)
+    @Nullable
+    @Replaces(oldName = "sstable_preemptive_open_interval_in_mb", converter = Converters.NEGATIVE_DATA_STORAGE_INT, deprecated = true)


Should't this keep including MEBIBYTES in the name, as in NEGATIVE_MEBIBYTES_DATA_STORAGE_INT or MEBIBYTES_CUSTOM_DATA_STORAGE_INT?

adelapena · 2022-07-28T14:29:20Z

src/java/org/apache/cassandra/config/DatabaseDescriptor.java


    public static int getSSTablePreemptiveOpenIntervalInMiB()
    {
+        if (conf.sstable_preemptive_open_interval == null)


I'd add some JavaDoc to the getter mentioning that it might return negative.

This info is in the JavaDoc for the JMX method but I guess it doesn't hurt to add it here too.

adelapena · 2022-07-28T14:29:48Z

src/java/org/apache/cassandra/config/DatabaseDescriptor.java

    public static void setSSTablePreemptiveOpenIntervalInMiB(int mib)
    {
-        conf.sstable_preemptive_open_interval = new DataStorageSpec.IntMebibytesBound(mib);
+        if (mib < 0)


I'd add some JavaDoc to this setter mentioning that it accepts negative values.

ekaterinadimitrova2 · 2022-07-28T16:31:20Z

@adelapena I addressed your feedback. The new tests pass locally, but I started full CI #1823 https://app.circleci.com/pipelines/github/ekaterinadimitrova2/cassandra?branch=17737-4.1-sstable_preemptive_open_interval_in_mb

ekaterinadimitrova2 · 2022-07-28T16:38:33Z

CI shows some issue with @nullable, I think I need to change the scope in build.xml for the annotation, I will work it now. I will let you know when I have the fix pushed

ekaterinadimitrova2 · 2022-07-28T18:00:13Z

com.google.code.findbugs needs to have default compile and not provided scope for this patch to work.
Looking into its license https://opensource.org/licenses/BSD-3-Clause
and reading here - https://www.apache.org/legal/resolved.html
I think this is fine. Do we need to bring it to the mailing list? I think only introducing brand new dependencies? WDYT?
CI - https://app.circleci.com/pipelines/github/ekaterinadimitrova2/cassandra?branch=17737-4.1-sstable_preemptive_open_interval_in_mb #1824 still running

jonmeredith · 2022-07-28T19:58:17Z

src/java/org/apache/cassandra/config/YamlConfigurationLoader.java

                public void set(Object object, Object value) throws Exception
                {
-                    if (value == null && get(object) != null)
+                    if (value == null && get(object) != null && !allowsNull)


It seems inconsistent to only require an Nullable annotation for configuration that can be set null, but is not defaulted to be null. What do you think about dropping the get(object) != null check, and applying @Nullable to all cases in Config that can be legitimately set to null?

I thought about that yesterday but my concern is that there are too many of those and they are not only from the new types and not only new ones...
Maybe we can open a follow up ticket only on trunk as an improvement? And add more testing, etc... I am afraid we are getting into another rabbit hole.
Also, it is good if we can keep this case as an exception and not make people think that it is a good practice. Why? Because if they start adding other default values and do not test properly that the null value is handled (and I am sure someone will do that) this can be reaaaally bad. What do you think?

Understand about rabbit holes. I'd be happy with opening a follow up JIRA and adding a comment referencing it in this method.

Ticket opened, I linked it to this ticket and added a TODO comment

adelapena · 2022-07-29T10:24:44Z

src/java/org/apache/cassandra/config/Converters.java

+    NEGATIVE_MEBIBYTES_DATA_STORAGE_INT(Integer.class, DataStorageSpec.IntMebibytesBound.class,
+                              o -> o < 0 ? null : new DataStorageSpec.IntMebibytesBound(o),
+                              o -> o == null ? -1 : o.toMebibytes()),


Trivial nit that can be addressed on commit: the parameters are misaligned after renaming the converter.

adelapena · 2022-07-29T11:25:33Z

The last changes look good to me. It seems that CI hasn't been started. Also we'd need to apply the changes to the patch for trunk.

jonmeredith

Looks good to me - assuming tests are passing and changes on this PR are propagated up to trunk

…acy_<version>_tuple" sstables. (apache#1758) Fix LegacySStableTest to only perform tuple checks on versions "ba" and higher because we are unable to generate tuple sstables in earlier versions.

ekaterinadimitrova2 added 2 commits July 27, 2022 09:41

Fix sstable_preemptive_open_interval_in_mb disabled value - backward …

9d051ce

…compatibility

DO NOT COMMIT CircleCI custom config

5cccd61

jonmeredith reviewed Jul 27, 2022

View reviewed changes

ekaterinadimitrova2 added 2 commits July 27, 2022 21:07

Fix starting cassandra with sstable_preemptive_open_interval null val…

9da01b7

…ue for disabled Fix Settings Virtual Table backward compatibility for properties that would be assigned null now instead of -1 or negatives

new line needed

a842fc7

ekaterinadimitrova2 added 2 commits July 27, 2022 23:38

Fix with Nullable annotation; fix from David Capwell

7ee8646

Formatting cassandra.yaml

08ffa6a

adelapena reviewed Jul 28, 2022

View reviewed changes

Address Andres feedback

aeea45b

Change the scope for findbugs from provided to default compile

ddcce07

jonmeredith reviewed Jul 28, 2022

View reviewed changes

Add todo for CASSANDRA-17785

2a2fbe4

adelapena reviewed Jul 29, 2022

View reviewed changes

jonmeredith approved these changes Jul 29, 2022

View reviewed changes

smiklosovic closed this Oct 22, 2022

		# Set sstable_preemptive_open_interval to null for disabled which is equivalent to the sstable_preemptive_open_interval_in_mb
		# being negative

	- `sstable_preemptive_open_interval_in_mb` being negatve for disabled is equivalent to `sstable_preemptive_open_interval`
	- `sstable_preemptive_open_interval_in_mb` being negative for disabled is equivalent to `sstable_preemptive_open_interval`

17737 4.1 sstable preemptive open interval in mb #1758

17737 4.1 sstable preemptive open interval in mb #1758

Uh oh!

Conversation

ekaterinadimitrova2 commented Jul 27, 2022

Uh oh!

jonmeredith left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonmeredith Jul 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ekaterinadimitrova2 Jul 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ekaterinadimitrova2 Jul 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ekaterinadimitrova2 commented Jul 27, 2022

Uh oh!

ekaterinadimitrova2 commented Jul 27, 2022

Uh oh!

ekaterinadimitrova2 commented Jul 28, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ekaterinadimitrova2 commented Jul 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ekaterinadimitrova2 commented Jul 28, 2022

Uh oh!

ekaterinadimitrova2 commented Jul 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonmeredith Jul 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ekaterinadimitrova2 Jul 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

jonmeredith Jul 27, 2022 •

edited

Loading

ekaterinadimitrova2 Jul 27, 2022 •

edited

Loading

ekaterinadimitrova2 Jul 27, 2022 •

edited

Loading

ekaterinadimitrova2 commented Jul 28, 2022 •

edited

Loading

ekaterinadimitrova2 commented Jul 28, 2022 •

edited

Loading

jonmeredith Jul 28, 2022 •

edited

Loading

ekaterinadimitrova2 Jul 28, 2022 •

edited

Loading