Patch submitted for review in CASSANDRA-17532
On Tue, 5 Apr 2022 at 8:56, Ekaterina Dimitrova
wrote:
>
> Thank you both!
>
> As my understanding is that it will be more involved to deal with the per
> version config loading and that is the reason it was not done yet but a
> flag was added, I suggest we opt in for the flag and revise again this
> after the release. Let’s add it to the other topics we discussed around the
> in-jvm framework.
>
> Two points to support my suggestion:
> - I just checked and there were no new in-jvm upgrade tests added since
> February, before that October and July last year in trunk. 9 months nothing
> added in the older branches. So it seems as a rare thing and just one flag
> to add to the test when writing it.
> - on trunk the old config (pre-15234) is read into the new types so by
> testing with it we actually exercise the whole path now - backward
> compatibility + load into the new types.
>
> Let me know if you have any concerns or questions.
> I will leave the discussion open until tomorrow and in case there are no
> concerns - I will assume lazy consensus and open a ticket to flip the flag.
>
> On Mon, 4 Apr 2022 at 13:41, David Capwell wrote:
>
>> I am cool with checking by default and disabling for tests that need it,
>> it may also make more sense to add an allow list so tests can explicitly
>> say which configs to ignore (though this sounds painful to implement).
>>
>>
>> On Apr 4, 2022, at 9:11 AM, Jon Meredith wrote:
>>
>> I think checking the validation rules as part of testing is important,
>> but making that a per-test concern for handling it may be frustrating.
>>
>> What do you think about extending in-JVM with a method that provides the
>> previous config and version and expects the Instance implementation to
>> upgrade it, and then leave the validation on? That would provide some
>> documentation of how deprecated/removed options are expected to be migrated.
>>
>> It might also be worthwhile extending the upgrade tests so that we can
>> also specify new configuration in addition to migrated configurated as the
>> instance is upgraded.
>>
>> On Mon, Apr 4, 2022 at 8:43 AM Ekaterina Dimitrova
>> wrote:
>>
>>> Hi everyone,
>>> In In-jvm tests there is a flag Constants.KEY_DTEST_API_CONFIG_CHECK to
>>> opt-in/out from config checks done in YamlConfigurationLoader#check().
>>> The upgrade tests are currently set to ignore that check in order to
>>> work around dealing with new properties in newer versions. What does this
>>> mean? My understanding is that the lowest version from which we start an
>>> upgrade test will load Config and use it for all versions. This means that
>>> our checks will fail because in newer versions we set new config in
>>> InstanceConfig that doesn't exist in the old version Config. If we opt in,
>>> the tests will start failing because we need to remove parameters.
>>>
>>> I suggest we opt in by default to the checks so people consciously add
>>> their config. What do I mean? Currently with the new types and names in
>>> trunk, we exercise the backward compatibility and we set the old config
>>> names and values that work with the previous versions and exercise the
>>> backward compatibility but If I add a new name to set for config, this will
>>> just be ignored silently and default Config is used. Test might even
>>> pass...This is risky.
>>> This was documented but I think the right course of action is to opt in
>>> for checks and people ignore the checks in upgrade tests when they are sure
>>> they add the right config and no typos, etc and they understand the
>>> implications. The situation since that check was added has changed and if
>>> we keep on adding more tests, I think this is important so we are sure we
>>> test properly.
>>>
>>> Please let me know if I am wrong in my understanding and what you think.
>>>
>>> Best regards,
>>> Ekaterina
>>>
>>
>>
