[DISCUSSION] Shall we update Constants.KEY_DTEST_API_CONFIG_CHECK to true by default for in-jvm upgrade tests?

[Ekaterina Dimitrova <e....@gmail.com>]

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

Re: [DISCUSSION] Shall we update Constants.KEY_DTEST_API_CONFIG_CHECK to true by default for in-jvm upgrade tests?

[Ekaterina Dimitrova <e....@gmail.com>]

I also opened CASSANDRA-17548 for removing the flag and extending the
in-jvm framework functionality, as we spoke, as a placeholder to flag we
need to work on better solution

On Mon, 11 Apr 2022 at 18:24, Ekaterina Dimitrova <e....@gmail.com>
wrote:

> Patch submitted for review in CASSANDRA-17532
>
> On Tue, 5 Apr 2022 at 8:56, Ekaterina Dimitrova <e....@gmail.com>
> 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 <dc...@apple.com> 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 <jm...@gmail.com> 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 <
>>> e.dimitrova@gmail.com> 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
>>>>
>>>
>>>

Re: [DISCUSSION] Shall we update Constants.KEY_DTEST_API_CONFIG_CHECK to true by default for in-jvm upgrade tests?

[Ekaterina Dimitrova <e....@gmail.com>]

Patch submitted for review in CASSANDRA-17532

On Tue, 5 Apr 2022 at 8:56, Ekaterina Dimitrova <e....@gmail.com>
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 <dc...@apple.com> 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 <jm...@gmail.com> 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 <e....@gmail.com>
>> 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
>>>
>>
>>

Re: [DISCUSSION] Shall we update Constants.KEY_DTEST_API_CONFIG_CHECK to true by default for in-jvm upgrade tests?

[Ekaterina Dimitrova <e....@gmail.com>]

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 <dc...@apple.com> 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 <jm...@gmail.com> 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 <e....@gmail.com>
> 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
>>
>
>

Re: [DISCUSSION] Shall we update Constants.KEY_DTEST_API_CONFIG_CHECK to true by default for in-jvm upgrade tests?

[David Capwell <dc...@apple.com>]

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 <jm...@gmail.com> 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 <e.dimitrova@gmail.com <ma...@gmail.com>> 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

Re: [DISCUSSION] Shall we update Constants.KEY_DTEST_API_CONFIG_CHECK to true by default for in-jvm upgrade tests?

[Jon Meredith <jm...@gmail.com>]

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 <e....@gmail.com>
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
>