CSHARP-1019: Add support for C* 4.1, DSE 6.9.x, and HCD releases to CI #615

SiyaoIsHiding · 2024-11-11T23:56:20Z

Got green here: https://jenkins-drivers.aws.dsinternal.org/blue/organizations/jenkins/drivers%2Fcsharp%2FJANE_TEST/detail/JANE_TEST/10/pipeline/

joao-r-reis · 2024-11-12T13:40:55Z

Jenkinsfile

@@ -129,13 +129,26 @@ CCM_CASSANDRA_VERSION=${DSE_FIXED_VERSION} # maintain for backwards compatibilit
 CCM_VERSION=${DSE_FIXED_VERSION}
 CCM_SERVER_TYPE=dse
 DSE_VERSION=${DSE_FIXED_VERSION}
-CCM_IS_DSE=true
+CCM_DISTRIBUTION=dse


There's another Jenkinsfile that needs this changes too probably. That jenkinsfile is responsible for daily/weekly builds.

joao-r-reis · 2024-11-12T13:42:44Z

Jenkinsfile

-                  'dse-6.8.30' // 6.8 current DataStax Enterprise
+                  'dse-6.8.30', // 6.8 current DataStax Enterprise
+                  'dse-6.9.3',
+                  'hcd-1.0.0'


We need to prune this a bit, the matrix is too long for a "per-commit" build. We can run all the versions we want on the daily/weekly builds.

Maybe we can use 3.11, 4.1, 5.0, dse-5.1, dse-6.9.3 and hcd-1.0.0 for per-commit

src/Cassandra.IntegrationTests/Core/SchemaMetadataTests.cs

src/Cassandra.IntegrationTests/TestClusterManagement/CcmCluster.cs

joao-r-reis · 2024-11-12T13:47:49Z

src/Cassandra.IntegrationTests/TestClusterManagement/TestClusterManager.cs

@@ -29,6 +29,7 @@ public static class TestClusterManager
        public const string DefaultKeyspaceName = "test_cluster_keyspace";
        private static ICcmProcessExecuter _executor;

+        private static readonly Version Version1Dot0 = new Version(1, 0);


We don't need this one do we?

joao-r-reis · 2024-11-12T13:50:05Z

src/Cassandra.IntegrationTests/TestClusterManagement/TestClusterManager.cs

@@ -70,15 +73,18 @@ public static Version CassandraVersion
                        // C* 3.0
                        return Version3Dot0;
                    }
-                    if (dseVersion < Version6Dot0)
+                    if (dseVersion <= Version6Dot9)


Hmm this is a tricky change. Technically DSE 6.x is in between 3.11 and 4.0 (it has some 4.0 features). This might result in some DSE 6.x tests not running because we declared the test as compatible with C* 4.0 and we relied on it working this way to run for DSE as well. Maybe try to check the total test count per server version before and after the changes to make sure we're not losing tests with these changes.

Before the fix, dse-6.7.17 net6: total tests 1145, skipped 108.
After the fix, dse-6.7.17 net6: total tests 1145, skipped 107.

Before fix, dse-6.8 net6, skipped 28
After fix, dse-6.8 net6, skipped 27

So after my fix, it actually skips fewer tests. I think it may be because my change in CheckCassandraVersion. Previously it maps C* version to DSE version when comparing, using GetDseVersionFromCassandraVersion. I changed it so it maps DSE version to C* version instead. I think it makes more sense, and align with other drivers better, too.

Before fix, dse-6.8 net6, skipped 28
After fix, dse-6.8 net6, skipped 27

Do you have a total number for dse-6.8 net6 similar to dse-6.7.17 net6?

I mean total number of tests, you wrote the total number of tests dse-6.7 but not dse-6.8

Oh the same, the number of total tests is 1145. This is the number of tests before skipping any, so it will be the same for any versions, before and after fix.

The one less skip count is about Should_FailFast_When_PreparedStatementIdChangesOnReprepare, because "This test relies on a bug that is fixed in this server version." I assume that test does not matter in this case.
Additionally, I just found out on the master branch, in [TestCassandraVersion(5, 0)],DSE 6.x has always map to C* 3.11. It's calling GetDseVersionFromCassandraVersion.
Mapping C* to DSE version is weird to me and brought a lot of confusion. For example, both with or without my fix, tests with [TestCassandraVersion(5, 0)] and the executing version of DSE-6.7.17 will run without being skipped.
Do we agree that

We should map DSE 6.x to C* 3.11, and

We change [TestCassandraVersion(x,x)] to map DSE version to C* version before it compares?

looks right to me assuming that we don't cause additional tests to be skipped or cause test failures

also looks right to me, it's weird that we're mapping C* to DSE version even when we're using OSS C* and the OSS C* test attribute. Probably a remnant from the time before the driver unification Idk

We should only try to map C*<->DSE versions if there's a mismatch between the attribute and the server type. If the attribute is DSE and the server is DSE no mapping is needed and the same thing is true if the attribute is OSS and the server is OSS.

Closing the loop here, without my fix dse-6.7.17 net6 skips 357 tests, with my fix it skips 355 tests. The only difference is:
Should_FailFast_When_PreparedStatementIdChangesOnReprepare which relies on a bug.
Should_CreateTable_WhenClusteringOrderAndCompactOptionsAreSet, the master branch skips this but it can actually pass.
I assume we are good mapping DSE 6.x to C* 3.11 now.

joao-r-reis · 2024-11-12T13:53:11Z

src/Cassandra.IntegrationTests/TestClusterManagement/TestClusterManager.cs

+        {
+            get
+            {
+                string distribution = Environment.GetEnvironmentVariable("CCM_DISTRIBUTION") ?? "cassandra";


Currently the test project is relying on checking for DSE_VERSION to see if the tests are running against DSE or not so we should also check TestClusterManager.IsDse here before falling back to cassandra.

Ok I see you also changed the implementation of IsDse. I still think we should check the DSE_VERSION env variable here before falling back to cassandra

joao-r-reis · 2024-11-12T13:56:40Z

src/Cassandra/SchemaParser.cs

@@ -840,8 +840,7 @@ public override Task<AggregateMetadata> GetAggregateAsync(string keyspaceName, s
                            StateFunction = row.GetValue<string>("state_func"),
                            FinalFunction = row.GetValue<string>("final_func"),
                            InitialCondition = row.GetValue<string>("initcond"),
-                            Deterministic = row.GetColumn("deterministic") != null &&
-                                            row.GetValue<bool>("deterministic"),
+                            Deterministic = row.GetColumn("deterministic") != null && row.GetValue<bool?>("deterministic").GetValueOrDefault(false),


Isn't this a server bug? Or is it working as intended to get null values here?

Based on the current status of the ticket, it looks like a bug: https://datastax.jira.com/browse/DSP-24606
What do we want to do here?

let's remove all production code changes from this PR and keep the test changes and make the failing HCD tests skip when it's HCD with a message that refers DSP-24606 (no full link since it's a private JIRA project).

The changes are trivial but I don't really want to do a release for the C# driver when this is something that will be fixed in HCD (and it's highly unlikely someone will run into this before an HCD fix is in place anyway).

update Jenkinsfile.scheduled

joao-r-reis · 2024-11-20T14:30:09Z

Apparently there's some git conflicts? Did you create your branch from an old version of master?

joao-r-reis · 2024-11-20T14:36:05Z

src/Cassandra.IntegrationTests/TestBase/TestCassandraVersion.cs

            int major, int minor, Comparison comparison = Comparison.GreaterThanOrEqualsTo, bool isOssRequired = false) : base(major, minor, comparison, isOssRequired)
        {
        }

-        public TestCassandraVersion(int major, int minor, int build, Comparison comparison = Comparison.GreaterThanOrEqualsTo, bool isOssRequired = false) : base(major, minor, build, comparison, isOssRequired)
+        public TestDseVersion(int major, int minor, int build, Comparison comparison = Comparison.GreaterThanOrEqualsTo, bool isOssRequired = false) : base(major, minor, build, comparison, isOssRequired)
        {
        }


I like the changes to these attributes, we just have to ensure there is no change to the number of tests that are actually running for a specific C*/DSE version

SiyaoIsHiding · 2024-11-21T04:24:30Z

Currently all the vector-related tests are marked with [TestBothServerVersions(5,0,6,9)] so they all fail with DSE-6.9, because those vector tests are with various subtypes, while DSE-6.9 only support vector. So I changed the existing vector tests to [TestCassandraVersion(5,0)] and I m adding some vector tests for DSE-6.9.

joao-r-reis · 2024-11-21T13:49:15Z

while DSE-6.9 only support vector

I don't get this, you mean it only supports vector of floats?

SiyaoIsHiding · 2024-11-21T16:45:17Z

Yes yes sorry I missed that. DSE-6.9 only supports vector of floats. I'm adding tests of vector of floats for DSE-6.9.

joao-r-reis · 2024-11-21T17:05:06Z

Yes yes sorry I missed that. DSE-6.9 only supports vector of floats. I'm adding tests of vector of floats for DSE-6.9.

Could also just add an if to the test to make sure all test cases are skipped except when subtype is float when it's DSE-6.9

joao-r-reis · 2024-11-21T18:01:09Z

Changes look good but it looks like we essentially doubled the matrix size for the "Per Commit" builds so we should try to trim it even further.

Let's do the following for Per Commit:

3.11
4.1
5.0
dse-5.1.35
dse-6.8.30
dse-6.9.3
hcd-1.0.0

Run all of these versions against net6.
Only 5.0, dse-6.9.3 and hcd-1.0.0 against mono
Only 4.1, dse-5.1.35 and dse-6.8.30 against net8

This will result in an increase of 2 builds (13) vs the number in master currently (11).

After doing this change and veirfying that CI is green you can merge this 👍

joao-r-reis · 2024-11-22T15:57:15Z

Jenkinsfile

            }
            axis {
              name 'SERVER_VERSION'
-              values '3.0', '5.0-beta1', 'dse-5.1.35', 'dse-6.8.30'
+              values '3.11', 4.1', 'dse-5.1.35', 'dse-6.8.30'


missing ' before 4.1

joao-r-reis · 2024-11-22T15:57:26Z

Jenkinsfile

@@ -463,7 +476,7 @@ pipeline {
            }
            axis {
              name 'SERVER_VERSION'
-              values 'dse-6.7.17', '3.11'
+              values '3.11', 5.0', 'dse-6.9.3', 'hcd-1.0.0'


missing ' before 5.0

SiyaoIsHiding · 2024-11-22T16:04:05Z

13 builds now

joao-r-reis · 2024-11-22T16:04:13Z

Matrix looks good now 👍

SiyaoIsHiding added 10 commits November 6, 2024 00:09

add 4.1 and 6.9 to matrix

ac5c0b0

,

f985c6f

fix yaml

c77a8ca

fix some update config didn't get through ccmbridge.updateconfig

059bbcd

add hcd

49e8122

add '''

eb9741b

BackendType

6e10968

fix compact storage

69b9eb3

Fix null values in UDF and UDA fields

3b05e38

short hand of nullable type

7eaff90

SiyaoIsHiding requested a review from joao-r-reis November 11, 2024 23:56

joao-r-reis reviewed Nov 12, 2024

View reviewed changes

src/Cassandra.IntegrationTests/Core/SchemaMetadataTests.cs Show resolved Hide resolved

joao-r-reis reviewed Nov 12, 2024

View reviewed changes

src/Cassandra.IntegrationTests/TestClusterManagement/CcmCluster.cs Show resolved Hide resolved

joao-r-reis reviewed Nov 12, 2024

View reviewed changes

SiyaoIsHiding added 5 commits November 18, 2024 12:06

ccm updateconfig merge to one line

eca06aa

update Jenkinsfile.scheduled

Remain backward compatibility to DSE_VERSION

8346a00

delete unused versions

fd61e10

refactor to only map DSE version to Cassandra version.

8b25d96

revert fix for DSP-24606

42e80d0

SiyaoIsHiding requested a review from joao-r-reis November 20, 2024 07:22

joao-r-reis reviewed Nov 20, 2024

View reviewed changes

SiyaoIsHiding added 3 commits November 20, 2024 10:04

add 6.7.17

7638e82

merge from master

be48ab7

adding back all versions

1b51a9f

Vector tests do not run with DSE

1232d7f

What if we map 6.x to 4.0

966a77c

SiyaoIsHiding added 2 commits November 21, 2024 09:20

tests count should be the same

6c58320

VectorFloatTest

1417a70

SiyaoIsHiding added 2 commits November 21, 2024 11:33

decrease matrix

3fddfa4

trim matrix

729b23f

joao-r-reis reviewed Nov 22, 2024

View reviewed changes

''

e6cda97

joao-r-reis merged commit aa4f36e into datastax:master Nov 22, 2024
0 of 3 checks passed

CSHARP-1019: Add support for C* 4.1, DSE 6.9.x, and HCD releases to CI #615

CSHARP-1019: Add support for C* 4.1, DSE 6.9.x, and HCD releases to CI #615

Uh oh!

Conversation

SiyaoIsHiding commented Nov 11, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SiyaoIsHiding Nov 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joao-r-reis Nov 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joao-r-reis commented Nov 20, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SiyaoIsHiding commented Nov 21, 2024

Uh oh!

joao-r-reis commented Nov 21, 2024

Uh oh!

SiyaoIsHiding commented Nov 21, 2024

Uh oh!

joao-r-reis commented Nov 21, 2024

Uh oh!

joao-r-reis commented Nov 21, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SiyaoIsHiding commented Nov 22, 2024

Uh oh!

joao-r-reis commented Nov 22, 2024

Uh oh!

Uh oh!

SiyaoIsHiding Nov 20, 2024 •

edited

Loading

joao-r-reis Nov 12, 2024 •

edited

Loading