You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cassandra.apache.org by "Caleb Rackliffe (Jira)" <ji...@apache.org> on 2020/08/18 19:30:00 UTC
[jira] [Comment Edited] (CASSANDRA-16054) Regression Test for
Compact Storage Upgrades When Table Has Clustering and Value Column
[ 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-unsubscribe@cassandra.apache.org
For additional commands, e-mail: commits-help@cassandra.apache.org