[ https://issues.apache.org/jira/browse/CASSANDRA-16054?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17180028#comment-17180028 ]
Caleb Rackliffe edited comment on CASSANDRA-16054 at 8/18/20, 7:29 PM: ----------------------------------------------------------------------- [~jwest] Made a first pass at review... *General Impressions* * Dropping COMPACT STORAGE seems like mostly a concern for the local storage engine, so my initial reaction to having even a 2-node cluster was that it might not be strictly necessary. Reading further, I see that we're looking at operational scenarios like schema not being in agreement, so I suppose a cluster makes sense if we're worried about that somehow confusing the coordinator. (I'm also not sure if we strictly need to query both coordinators post-drop, but perhaps that would only be a real problem if we were executing many more queries.) If this were a unit test, I might try to isolate something like the behavior w/ a schema mismatch in its own test, but I know our threshold for that kind of thing is higher when spinning up a new cluster (even in-JVM) isn't free :) * In {{testDropCompactWithClusteringAndValueColumn}}, it might be interesting to test a range query on the last clustering key and include a query or two that return no results by design. * In {{testDropCompactWithClusteringAndValueColumnWithDeletesAndWrites}}, was the intent to write new partitions, or just overwrite some of the partitions from the first set of 10? Testing a range delete might be interesting, but we might need to add a second clustering key? *Minor Things * * {{import org.apache.cassandra.distributed.shared.NetworkTopology}} looks unused in {{DelegatingInvokableInstance}} * Were the field visibility changes in {{AbstractCluster}} and {{DelegatingInvokableInstance}} necessary? * {{preUpgradeResults}} can be final in {{DropCompactTestHelper}} * {{DropCompactTestHelper}} could be named something like {{ResultRecorder}} (class) / {{recorder}} (local). (My guess is that this construct might end up being used more and more as we add upgrade tests?) It might even make sense to move things like {{validateResults()}} into it, and you'd have the beginnings of a "differ" / "validator" that you can populate however you want and then throw other clusters at. * Both tests issue coordinator queries at CL=ONE rather than using {{executeInternal()}}. Are we guaranteed that the former can't hit the wrong node? * We might be able to factor out the {{upgradesstables}} call from the two new tests. was (Author: maedhroz): [~jwest] Made a first pass at review... *General Impressions* * Dropping COMPACT STORAGE seems like mostly a concern for the local storage engine, so my initial reaction to having even a 2-node cluster was that it might not be strictly necessary. Reading further, I see that we're looking at operational scenarios like schema not being in agreement, so I suppose a cluster makes sense if we're worried about that somehow confusing the coordinator. (I'm also not sure if we strictly need to query both coordinators post-drop, but perhaps that would only be a real problem if we were executing many more queries.) * In {{testDropCompactWithClusteringAndValueColumn}}, it might be interesting to test a range query on the last clustering key and include a query or two that return no results by design. * In {{testDropCompactWithClusteringAndValueColumnWithDeletesAndWrites}}, was the intent to write new partitions, or just overwrite some of the partitions from the first set of 10? Testing a range delete might be interesting, but we might need to add a second clustering key? *Minor Things * * {{import org.apache.cassandra.distributed.shared.NetworkTopology}} looks unused in {{DelegatingInvokableInstance}} * Were the field visibility changes in {{AbstractCluster}} and {{DelegatingInvokableInstance}} necessary? * {{preUpgradeResults}} can be final in {{DropCompactTestHelper}} * {{DropCompactTestHelper}} could be named something like {{ResultRecorder}} (class) / {{recorder}} (local). (My guess is that this construct might end up being used more and more as we add upgrade tests?) It might even make sense to move things like {{validateResults()}} into it, and you'd have the beginnings of a "differ" / "validator" that you can populate however you want and then throw other clusters at. * Both tests issue coordinator queries at CL=ONE rather than using {{executeInternal()}}. Are we guaranteed that the former can't hit the wrong node? * We might be able to factor out the {{upgradesstables}} call from the two new tests. > Regression Test for Compact Storage Upgrades When Table Has Clustering and > Value Column > ---------------------------------------------------------------------------------------- > > Key: CASSANDRA-16054 > URL: https://issues.apache.org/jira/browse/CASSANDRA-16054 > Project: Cassandra > Issue Type: Task > Components: Legacy/CQL > Reporter: Jordan West > Assignee: Jordan West > Priority: Normal > > Add a regression test that shows that dropping compact storage on tables that > have clustering columns and a value column defined are safe after upgrading > sstables in 3.0 -- This message was sent by Atlassian Jira (v8.3.4#803005) --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org