CASSANDRA-18506: Upgrade tests cleanup

[jakubzytka]

This PR cleans up and simplifies the in-jvm upgrade tests machinery.

The assumptions used in this PR:

• only upgrades touching the CURRENT version should be run during testing
• the API for defining which upgrades to run should not be imperative ("do this and this upgrade"), but instead should be a declaration of test case applicability

Specific changes are:

• introduce new API in UpgradeTestBase that clearly expresses test case applicability
• deprecate the old, confusing UpgradeTestBase API, and express it in terms of the new one
• remote old, confusing, unused UpgradeTestBase API
• improve javadoc for UpgradeTestBase API
• remove List usage for target version in TestVersions, as these lists always had precisely one element
• fix CompactionHistorySystemTableUpgradeTest executing the same cases multiple tiems
• remove obsolete test cases in MixedModeFrom3UnloggedBatchTest and MixedModeFrom3LoggedBatchTest
• removal of duplicate test cases with V3X in test name

[michaelsembwever]

can you please start with this commit: https://github.com/apache/cassandra/compare/trunk...thelastpickle:cassandra:mck/18506/trunk

this fixes the .contains(..) bug, and should make things a lot clearer to understand, without having to touch method signatures (which touches all the other classes). starting off like this will help the review process progress (we can re-evaluate the need to improve method names later on in the review process…)

[jakubzytka]

can you please start with this commit: trunk...thelastpickle:cassandra:mck/18506/trunk this fixes the .contains(..) bug,

Are you sure this commit fixes the .contains() bug? First of all, the edgeTouchesTarget(Semver from, Semver to, Semver target) has special handling for patch etc. versions. This behaviour is never explained. Furthermore, this function may be buggy. Consider a test which defines its applicability from version X.Y.Z to some version X+1.0.0. If CURRENT is X.Y.Z-1 then this function returns true even though the test author specifically prohibited running it. Possibly, this function builds on some assumption that is never explicitly stated, like: "the only valid lower applicability bounds are non-patch etc. versions" I'd recommend using an assertion to express assumptions under which the function is valid.

should make things a lot clearer to understand,

the commit in question adds more complexity to already complex code and doesn't fix existing inconsistencies between javadocs or the code. I don't see it as a step in the right direction, considering that a simpler alternative exists in this PR.

without having to touch method signatures (which touches all the other classes)

would you kindly elaborate? My PR doesn't change the signatures of the existing public API.

starting off like this will help the review process progress

I don't understand what you mean. As already mentioned, the commit 9e32b906fb2fe822be39005ba40de882407e7848 adds additional code, whereas my PR mostly removes code. I don't see how could these two approaches meet. Unless, obviously, my change is based on some invalid assumptions, or doesn't take into account additional assumptions. I admit I struggled with that as I was unable to find documentation of these assumptions.

Is there any particular concern why reviewing this PR would be in any way difficult? If so, that's perhaps a reason to improve it. I tried to simplify the code as much as possible. Specifically, I'm removing complex code and replacing it with a simpler alternative.

[michaelsembwever]

I'm keen to address the .contains(…) bug, using the commit provided, before moving forward. This will ensure all assumptions about the correct code and naming used is understood.

[dcapwell]

would you kindly elaborate? My PR doesn't change the signatures of the existing public API.

this changes public APIs, which increases the scope of this patch.

• upgrades is now applicableVersionsRange
• upgradesToCurrentFrom is now minimalApplicableVersion

this adds a lot more noise to the patch, making it hard to see what is a bug fix, and what is a personal style change.

[michaelsembwever]

@jakubzytka, as David says, patches require a minimal approach in the project, along with meeting the style.

I know I led you down the garden path by encouraging improvements to UpgradeTestBase to make it easier to understand. apologies for that.

The .contains(..) bug is now fixed, it is in the .edgeTouchesTarget(..) method. And the existing .contains(..) method is renamed to .rangeCoversTarget(..).

With that in place, you can see that I've only then needed to change the parameter names to the method: upgradesToCurrentFrom(..), upgradesTo(..), upgradesFrom(..) and upgrades(..).

This should help demonstrate my earlier points that

• no existing apidoc or code comments were wrong, and
• upgrade and upgradeVersion needs to be lists.

To go into more detail…

First of all, the edgeTouchesTarget(Semver from, Semver to, Semver target) has special handling for patch etc. versions.

The supported upgrade paths do not go to the granularity of patch versions.

Furthermore, this function may be buggy. Consider a test which defines its applicability from version X.Y.Z

This is a valid point, but is not a problem today and out-of-scope. AFAIK no one is passing patch-number explicit versions to these methods. I agree that an assertion that we don't (yet) support patch-specific bounds would be appropriate.

-- I've completed the patch for the ticket with an additional commit on my branch.

[jakubzytka]

would you kindly elaborate? My PR doesn't change the signatures of the existing public API.

this changes public APIs, which increases the scope of this patch.

• upgrades is now applicableVersionsRange
• upgradesToCurrentFrom is now minimalApplicableVersion

this adds a lot more noise to the patch, making it hard to see what is a bug fix, and what is a personal style change.

The above statements are not true. Both upgrades and upgradesToCurrentFrom are present and offer the same semantics as previously.

[michaelsembwever]

This is a valid point, but is not a problem today and out-of-scope. AFAIK no one is passing patch-number explicit versions to these methods. I agree that an assertion that we don't (yet) support patch-specific bounds would be appropriate.

Instead of adding an assertion, I've added the apidoc. It is ok if patch versions are passed to the the edgeTouchesTarget(..) method, it is intentionally only compares majors and minors.

The potential bug you describe won't happen, because it is not this method that applies the bounds. That happens with the vertices.subSet(..) call.