cassandra
cassandra copied to clipboard
apache
17737 4.1 sstable preemptive open interval in mb
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()),
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
@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 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
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
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
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.
