adelapena commented on code in PR #2125:
URL: https://github.com/apache/cassandra/pull/2125#discussion_r1105748371
##########
test/distributed/org/apache/cassandra/distributed/upgrade/MixedModeColumnMaskingTest.java:
##########
@@ -104,10 +111,21 @@ private static void assertFails(ICoordinator coordinator,
String schemaQuery, St
.hasMessageContaining(expectedMessage);
}
- private void assertColumnValue(UpgradeableCluster cluster, String table,
String column, Object value)
+ private static void assertColumnIsMasked(UpgradeableCluster cluster,
String table, String column)
Review Comment:
The purpose of the test is only verifying that functions cannot be attached
to table columns during a rolling upgrade involving nodes that don't include
DDM. So I think the important part is checking that the schema-altering queries
that add a mask are rejected on mixed clusters.
Verifying that the masks are actually created is an extra, and I tried to
achieve it in the simpler possible way. In CASSANDRA-18069 it was easy enough
to just run a query. However, this is trickier to do with the dtests machinery
when we have permissions. We would need to create users and CQL driver
sessions, and switch between sessions to use a superuser for the
schema-altering queries and an unprivileged user for the `SELECT` queries.
Since we have two nodes in different versions and we want the driver to
target specific nodes, we would need to write a custom `LoadBalancingPolicy`
that only connects to one node. Also, since permission propagation takes a time
we would need a spin assert for every `SELECT` query.
The combination of multiple sessions and multiple spin asserts would make
the test significantly slower. Since upgrade dtests are quite prone to timeouts
and OOMs, we would probably need to split the test with at least one class per
upgrade path.
These changes add quite a bit of complexity to what I think is the less
important part of the test. So I decided to go for the simpler approach of just
checking that the tested schema changes are applied, and assume that if the
schema is good then the behaviour associated to it is also good. After all,
that part should be covered by the regular unit tests.
Do you think it makes sense, or should we go for an upgrade dtest testing
also `SELECT` queries?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
