Patch submitted for review in CASSANDRA-17532 On Tue, 5 Apr 2022 at 8:56, Ekaterina Dimitrova <e.dimitr...@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 <dcapw...@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 <jmeredit...@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.dimitr...@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 >>> >> >>
