Fix is_dense recalculation for Thrift-updated tables patch by Aleksey Yeschenko; reviewed by Sylvain Lebresne for CASSANDRA-11502
Project: http://git-wip-us.apache.org/repos/asf/cassandra/repo Commit: http://git-wip-us.apache.org/repos/asf/cassandra/commit/e5c40278 Tree: http://git-wip-us.apache.org/repos/asf/cassandra/tree/e5c40278 Diff: http://git-wip-us.apache.org/repos/asf/cassandra/diff/e5c40278 Branch: refs/heads/cassandra-3.0 Commit: e5c40278001bf3a9582085a58941e5f4765f118c Parents: 3db30aa Author: Aleksey Yeschenko <alek...@apache.org> Authored: Fri Apr 1 17:36:14 2016 +0100 Committer: Aleksey Yeschenko <alek...@apache.org> Committed: Wed Apr 27 17:47:29 2016 +0100 ---------------------------------------------------------------------- CHANGES.txt | 3 +- .../cql3/statements/AlterTableStatement.java | 2 +- .../cql3/statements/AlterTypeStatement.java | 2 +- .../cql3/statements/CreateIndexStatement.java | 2 +- .../cql3/statements/CreateTriggerStatement.java | 2 +- .../cql3/statements/DropIndexStatement.java | 2 +- .../cql3/statements/DropTriggerStatement.java | 2 +- .../cassandra/schema/LegacySchemaTables.java | 10 +--- .../cassandra/service/MigrationManager.java | 8 +-- .../cassandra/thrift/CassandraServer.java | 2 +- .../cassandra/thrift/ThriftConversion.java | 24 +++++++- .../config/LegacySchemaTablesTest.java | 60 +++++++++++++++++++- .../org/apache/cassandra/schema/DefsTest.java | 14 ++--- .../cassandra/triggers/TriggersSchemaTest.java | 4 +- 14 files changed, 103 insertions(+), 34 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/cassandra/blob/e5c40278/CHANGES.txt ---------------------------------------------------------------------- diff --git a/CHANGES.txt b/CHANGES.txt index e8a301a..3641816 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,9 +1,10 @@ 2.2.7 + * Fix is_dense recalculation for Thrift-updated tables (CASSANDRA-11502) * Remove unnescessary file existence check during anticompaction (CASSANDRA-11660) * Add missing files to debian packages (CASSANDRA-11642) * Avoid calling Iterables::concat in loops during ModificationStatement::getFunctions (CASSANDRA-11621) * cqlsh: COPY FROM should use regular inserts for single statement batches and - report errors correctly if workers processes crash on initialization (CASSANDRA-11474) + report errors correctly if workers processes crash on initialization (CASSANDRA-11474) * Always close cluster with connection in CqlRecordWriter (CASSANDRA-11553) Merged from 2.1: * cqlsh COPY FROM fails for null values with non-prepared statements (CASSANDRA-11631) http://git-wip-us.apache.org/repos/asf/cassandra/blob/e5c40278/src/java/org/apache/cassandra/cql3/statements/AlterTableStatement.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/cql3/statements/AlterTableStatement.java b/src/java/org/apache/cassandra/cql3/statements/AlterTableStatement.java index 63a53fa..f4a7b39 100644 --- a/src/java/org/apache/cassandra/cql3/statements/AlterTableStatement.java +++ b/src/java/org/apache/cassandra/cql3/statements/AlterTableStatement.java @@ -284,7 +284,7 @@ public class AlterTableStatement extends SchemaAlteringStatement break; } - MigrationManager.announceColumnFamilyUpdate(cfm, false, isLocalOnly); + MigrationManager.announceColumnFamilyUpdate(cfm, isLocalOnly); return true; } http://git-wip-us.apache.org/repos/asf/cassandra/blob/e5c40278/src/java/org/apache/cassandra/cql3/statements/AlterTypeStatement.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/cql3/statements/AlterTypeStatement.java b/src/java/org/apache/cassandra/cql3/statements/AlterTypeStatement.java index 6459e6b..9203cf9 100644 --- a/src/java/org/apache/cassandra/cql3/statements/AlterTypeStatement.java +++ b/src/java/org/apache/cassandra/cql3/statements/AlterTypeStatement.java @@ -113,7 +113,7 @@ public abstract class AlterTypeStatement extends SchemaAlteringStatement for (ColumnDefinition def : copy.allColumns()) modified |= updateDefinition(copy, def, toUpdate.keyspace, toUpdate.name, updated); if (modified) - MigrationManager.announceColumnFamilyUpdate(copy, false, isLocalOnly); + MigrationManager.announceColumnFamilyUpdate(copy, isLocalOnly); } // Other user types potentially using the updated type http://git-wip-us.apache.org/repos/asf/cassandra/blob/e5c40278/src/java/org/apache/cassandra/cql3/statements/CreateIndexStatement.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/cql3/statements/CreateIndexStatement.java b/src/java/org/apache/cassandra/cql3/statements/CreateIndexStatement.java index edc092d..d93c0a7 100644 --- a/src/java/org/apache/cassandra/cql3/statements/CreateIndexStatement.java +++ b/src/java/org/apache/cassandra/cql3/statements/CreateIndexStatement.java @@ -191,7 +191,7 @@ public class CreateIndexStatement extends SchemaAlteringStatement cd.setIndexName(indexName); cfm.addDefaultIndexNames(); - MigrationManager.announceColumnFamilyUpdate(cfm, false, isLocalOnly); + MigrationManager.announceColumnFamilyUpdate(cfm, isLocalOnly); return true; } http://git-wip-us.apache.org/repos/asf/cassandra/blob/e5c40278/src/java/org/apache/cassandra/cql3/statements/CreateTriggerStatement.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/cql3/statements/CreateTriggerStatement.java b/src/java/org/apache/cassandra/cql3/statements/CreateTriggerStatement.java index 6ebe0d3..ef2f263 100644 --- a/src/java/org/apache/cassandra/cql3/statements/CreateTriggerStatement.java +++ b/src/java/org/apache/cassandra/cql3/statements/CreateTriggerStatement.java @@ -78,7 +78,7 @@ public class CreateTriggerStatement extends SchemaAlteringStatement { cfm.addTriggerDefinition(triggerDefinition); logger.info("Adding trigger with name {} and class {}", triggerName, triggerClass); - MigrationManager.announceColumnFamilyUpdate(cfm, false, isLocalOnly); + MigrationManager.announceColumnFamilyUpdate(cfm, isLocalOnly); return true; } return false; http://git-wip-us.apache.org/repos/asf/cassandra/blob/e5c40278/src/java/org/apache/cassandra/cql3/statements/DropIndexStatement.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/cql3/statements/DropIndexStatement.java b/src/java/org/apache/cassandra/cql3/statements/DropIndexStatement.java index c6c0244..0d33e57 100644 --- a/src/java/org/apache/cassandra/cql3/statements/DropIndexStatement.java +++ b/src/java/org/apache/cassandra/cql3/statements/DropIndexStatement.java @@ -97,7 +97,7 @@ public class DropIndexStatement extends SchemaAlteringStatement CFMetaData updatedCfm = updateCFMetadata(cfm); indexedCF = updatedCfm.cfName; - MigrationManager.announceColumnFamilyUpdate(updatedCfm, false, isLocalOnly); + MigrationManager.announceColumnFamilyUpdate(updatedCfm, isLocalOnly); return true; } http://git-wip-us.apache.org/repos/asf/cassandra/blob/e5c40278/src/java/org/apache/cassandra/cql3/statements/DropTriggerStatement.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/cql3/statements/DropTriggerStatement.java b/src/java/org/apache/cassandra/cql3/statements/DropTriggerStatement.java index e3db1e1..8267b4e 100644 --- a/src/java/org/apache/cassandra/cql3/statements/DropTriggerStatement.java +++ b/src/java/org/apache/cassandra/cql3/statements/DropTriggerStatement.java @@ -63,7 +63,7 @@ public class DropTriggerStatement extends SchemaAlteringStatement if (cfm.removeTrigger(triggerName)) { logger.info("Dropping trigger with name {}", triggerName); - MigrationManager.announceColumnFamilyUpdate(cfm, false, isLocalOnly); + MigrationManager.announceColumnFamilyUpdate(cfm, isLocalOnly); return true; } if (!ifExists) http://git-wip-us.apache.org/repos/asf/cassandra/blob/e5c40278/src/java/org/apache/cassandra/schema/LegacySchemaTables.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/schema/LegacySchemaTables.java b/src/java/org/apache/cassandra/schema/LegacySchemaTables.java index 0ef9852..8d5bf4f 100644 --- a/src/java/org/apache/cassandra/schema/LegacySchemaTables.java +++ b/src/java/org/apache/cassandra/schema/LegacySchemaTables.java @@ -908,8 +908,7 @@ public class LegacySchemaTables public static Mutation makeUpdateTableMutation(KSMetaData keyspace, CFMetaData oldTable, CFMetaData newTable, - long timestamp, - boolean fromThrift) + long timestamp) { Mutation mutation = makeCreateKeyspaceMutation(keyspace, timestamp, false); @@ -920,14 +919,7 @@ public class LegacySchemaTables // columns that are no longer needed for (ColumnDefinition column : columnDiff.entriesOnlyOnLeft().values()) - { - // Thrift only knows about the REGULAR ColumnDefinition type, so don't consider other type - // are being deleted just because they are not here. - if (fromThrift && column.kind != ColumnDefinition.Kind.REGULAR) - continue; - dropColumnFromSchemaMutation(oldTable, column, timestamp, mutation); - } // newly added columns for (ColumnDefinition column : columnDiff.entriesOnlyOnRight().values()) http://git-wip-us.apache.org/repos/asf/cassandra/blob/e5c40278/src/java/org/apache/cassandra/service/MigrationManager.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/service/MigrationManager.java b/src/java/org/apache/cassandra/service/MigrationManager.java index ec7448a..f7a2a48 100644 --- a/src/java/org/apache/cassandra/service/MigrationManager.java +++ b/src/java/org/apache/cassandra/service/MigrationManager.java @@ -346,12 +346,12 @@ public class MigrationManager announce(LegacySchemaTables.makeCreateKeyspaceMutation(ksm, FBUtilities.timestampMicros()), announceLocally); } - public static void announceColumnFamilyUpdate(CFMetaData cfm, boolean fromThrift) throws ConfigurationException + public static void announceColumnFamilyUpdate(CFMetaData cfm) throws ConfigurationException { - announceColumnFamilyUpdate(cfm, fromThrift, false); + announceColumnFamilyUpdate(cfm, false); } - public static void announceColumnFamilyUpdate(CFMetaData cfm, boolean fromThrift, boolean announceLocally) throws ConfigurationException + public static void announceColumnFamilyUpdate(CFMetaData cfm, boolean announceLocally) throws ConfigurationException { cfm.validate(); @@ -363,7 +363,7 @@ public class MigrationManager oldCfm.validateCompatility(cfm); logger.info(String.format("Update table '%s/%s' From %s To %s", cfm.ksName, cfm.cfName, oldCfm, cfm)); - announce(LegacySchemaTables.makeUpdateTableMutation(ksm, oldCfm, cfm, FBUtilities.timestampMicros(), fromThrift), announceLocally); + announce(LegacySchemaTables.makeUpdateTableMutation(ksm, oldCfm, cfm, FBUtilities.timestampMicros()), announceLocally); } public static void announceTypeUpdate(UserType updatedType, boolean announceLocally) http://git-wip-us.apache.org/repos/asf/cassandra/blob/e5c40278/src/java/org/apache/cassandra/thrift/CassandraServer.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/thrift/CassandraServer.java b/src/java/org/apache/cassandra/thrift/CassandraServer.java index 694a36a..36664ae 100644 --- a/src/java/org/apache/cassandra/thrift/CassandraServer.java +++ b/src/java/org/apache/cassandra/thrift/CassandraServer.java @@ -1661,7 +1661,7 @@ public class CassandraServer implements Cassandra.Iface if (!oldCfm.getTriggers().equals(cfm.getTriggers())) state().ensureIsSuper("Only superusers are allowed to add or remove triggers."); - MigrationManager.announceColumnFamilyUpdate(cfm, true); + MigrationManager.announceColumnFamilyUpdate(cfm); return Schema.instance.getVersion().toString(); } catch (RequestValidationException e) http://git-wip-us.apache.org/repos/asf/cassandra/blob/e5c40278/src/java/org/apache/cassandra/thrift/ThriftConversion.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/thrift/ThriftConversion.java b/src/java/org/apache/cassandra/thrift/ThriftConversion.java index adb925e..04eae38 100644 --- a/src/java/org/apache/cassandra/thrift/ThriftConversion.java +++ b/src/java/org/apache/cassandra/thrift/ThriftConversion.java @@ -223,16 +223,33 @@ public class ThriftConversion if (hasKeyAlias) defs.add(ColumnDefinition.partitionKeyDef(cf_def.keyspace, cf_def.name, cf_def.key_alias, keyValidator, null)); + // for Thrift updates, we should be calculating denseness from just the regular columns & comparator + boolean isDense = CFMetaData.calculateIsDense(fullRawComparator, defs); + // Now add any CQL metadata that we want to copy, skipping the keyAlias if there was one for (ColumnDefinition def : previousCQLMetadata) { - // isPartOfCellName basically means 'is not just a CQL metadata' - if (def.isPartOfCellName()) + // skip all pre-existing REGULAR columns + if (def.kind == ColumnDefinition.Kind.REGULAR) continue; + // skip previous PARTITION_KEY column def if key_alias has been set by this update already (overwritten) if (def.kind == ColumnDefinition.Kind.PARTITION_KEY && hasKeyAlias) continue; + // the table switched from DENSE to SPARSE by adding one or more REGULAR columns; + // in this case we should now drop the COMPACT_VALUE column + if (def.kind == ColumnDefinition.Kind.COMPACT_VALUE && !isDense) + continue; + + // skip CLUSTERING_COLUMN column(s) of a sparse table, if: + // a) this is a Standard columnfamily *OR* b) it's a Super columnfamily and the second (subcolumn) component; + // in other words, only keep the clustering column in sparse tables if it's the first (super) component + // of a super column family + if (def.kind == ColumnDefinition.Kind.CLUSTERING_COLUMN && !isDense) + if (cfType == ColumnFamilyType.Standard || def.position() != 0) + continue; + defs.add(def); } @@ -242,7 +259,8 @@ public class ThriftConversion if (cfId == null) cfId = UUIDGen.getTimeUUID(); - CFMetaData newCFMD = new CFMetaData(cf_def.keyspace, cf_def.name, cfType, comparator, cfId); + // set isDense now so that it doesn't get re-calculated incorrectly later in rebuild() b/c of defined clusterings + CFMetaData newCFMD = new CFMetaData(cf_def.keyspace, cf_def.name, cfType, comparator, cfId).isDense(isDense); newCFMD.addAllColumnDefinitions(defs); http://git-wip-us.apache.org/repos/asf/cassandra/blob/e5c40278/test/unit/org/apache/cassandra/config/LegacySchemaTablesTest.java ---------------------------------------------------------------------- diff --git a/test/unit/org/apache/cassandra/config/LegacySchemaTablesTest.java b/test/unit/org/apache/cassandra/config/LegacySchemaTablesTest.java index 3642e7a..f630c88 100644 --- a/test/unit/org/apache/cassandra/config/LegacySchemaTablesTest.java +++ b/test/unit/org/apache/cassandra/config/LegacySchemaTablesTest.java @@ -1,4 +1,4 @@ -/** +/* * Licensed to the Apache Software Foundation (ASF) under one * or more contributor license agreements. See the NOTICE file * distributed with this work for additional information @@ -19,18 +19,23 @@ package org.apache.cassandra.config; import java.util.ArrayList; +import java.util.Collections; import java.util.List; import java.util.HashMap; import java.util.HashSet; +import com.google.common.collect.Iterables; + import org.apache.cassandra.SchemaLoader; import org.apache.cassandra.db.*; import org.apache.cassandra.db.marshal.AsciiType; +import org.apache.cassandra.db.marshal.BytesType; import org.apache.cassandra.db.marshal.UTF8Type; import org.apache.cassandra.exceptions.ConfigurationException; import org.apache.cassandra.io.compress.*; import org.apache.cassandra.locator.SimpleStrategy; import org.apache.cassandra.schema.LegacySchemaTables; +import org.apache.cassandra.service.MigrationManager; import org.apache.cassandra.service.StorageService; import org.apache.cassandra.thrift.CfDef; import org.apache.cassandra.thrift.ColumnDef; @@ -43,11 +48,16 @@ import org.junit.BeforeClass; import org.junit.Test; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; public class LegacySchemaTablesTest { private static final String KEYSPACE1 = "CFMetaDataTest1"; private static final String CF_STANDARD1 = "Standard1"; + private static final String CF_STANDARD2 = "Standard2"; private static List<ColumnDef> columnDefs = new ArrayList<ColumnDef>(); @@ -73,6 +83,54 @@ public class LegacySchemaTablesTest } @Test + public void testIsDenseRecalculation() + { + // 1.a start with a dense CF + CfDef cfDef0 = new CfDef().setDefault_validation_class(BytesType.class.getCanonicalName()) + .setComparator_type(UTF8Type.class.getCanonicalName()) + .setColumn_metadata(Collections.<ColumnDef>emptyList()) + .setKeyspace(KEYSPACE1) + .setName(CF_STANDARD2); + CFMetaData cfm0 = ThriftConversion.fromThrift(cfDef0); + MigrationManager.announceNewColumnFamily(cfm0, true); + + // 1.b validate that the cf is dense, has a single compact value and a clustering column, and no regulars + CFMetaData current = Schema.instance.getCFMetaData(KEYSPACE1, CF_STANDARD2); + assertTrue(current.getIsDense()); + assertNotNull(current.compactValueColumn()); + assertEquals(0, Iterables.size(current.regularAndStaticColumns())); + assertEquals(1, current.clusteringColumns().size()); + + // 2.a add a column to the table + CfDef cfDef1 = ThriftConversion.toThrift(current); + List<ColumnDef> colDefs = + Collections.singletonList(new ColumnDef(ByteBufferUtil.bytes("col1"), AsciiType.class.getCanonicalName())); + cfDef1.setColumn_metadata(colDefs); + CFMetaData cfm1 = ThriftConversion.fromThriftForUpdate(cfDef1, current); + MigrationManager.announceColumnFamilyUpdate(cfm1, true); + + // 2.b validate that the cf is sparse now, had no compact value column or clustering column, and 1 regular + current = Schema.instance.getCFMetaData(KEYSPACE1, CF_STANDARD2); + assertFalse(current.getIsDense()); + assertNull(current.compactValueColumn()); + assertEquals(1, Iterables.size(current.regularAndStaticColumns())); + assertEquals(0, current.clusteringColumns().size()); + + // 3.a remove the column + CfDef cfDef2 = ThriftConversion.toThrift(current); + cfDef2.setColumn_metadata(Collections.<ColumnDef>emptyList()); + CFMetaData cfm2 = ThriftConversion.fromThriftForUpdate(cfDef2, current); + MigrationManager.announceColumnFamilyUpdate(cfm2, true); + + // 3.b validate that the cf is dense, has a single compact value and a clustering column, and no regulars + current = Schema.instance.getCFMetaData(KEYSPACE1, CF_STANDARD2); + assertTrue(current.getIsDense()); + assertNotNull(current.compactValueColumn()); + assertEquals(0, Iterables.size(current.regularAndStaticColumns())); + assertEquals(1, current.clusteringColumns().size()); + } + + @Test public void testThriftConversion() throws Exception { CfDef cfDef = new CfDef().setDefault_validation_class(AsciiType.class.getCanonicalName()) http://git-wip-us.apache.org/repos/asf/cassandra/blob/e5c40278/test/unit/org/apache/cassandra/schema/DefsTest.java ---------------------------------------------------------------------- diff --git a/test/unit/org/apache/cassandra/schema/DefsTest.java b/test/unit/org/apache/cassandra/schema/DefsTest.java index 302878a..ecf6709 100644 --- a/test/unit/org/apache/cassandra/schema/DefsTest.java +++ b/test/unit/org/apache/cassandra/schema/DefsTest.java @@ -462,22 +462,22 @@ public class DefsTest // test valid operations. newCfm.comment("Modified comment"); - MigrationManager.announceColumnFamilyUpdate(newCfm, false); // doesn't get set back here. + MigrationManager.announceColumnFamilyUpdate(newCfm); // doesn't get set back here. newCfm.readRepairChance(0.23); - MigrationManager.announceColumnFamilyUpdate(newCfm, false); + MigrationManager.announceColumnFamilyUpdate(newCfm); newCfm.gcGraceSeconds(12); - MigrationManager.announceColumnFamilyUpdate(newCfm, false); + MigrationManager.announceColumnFamilyUpdate(newCfm); newCfm.defaultValidator(UTF8Type.instance); - MigrationManager.announceColumnFamilyUpdate(newCfm, false); + MigrationManager.announceColumnFamilyUpdate(newCfm); newCfm.minCompactionThreshold(3); - MigrationManager.announceColumnFamilyUpdate(newCfm, false); + MigrationManager.announceColumnFamilyUpdate(newCfm); newCfm.maxCompactionThreshold(33); - MigrationManager.announceColumnFamilyUpdate(newCfm, false); + MigrationManager.announceColumnFamilyUpdate(newCfm); // can't test changing the reconciler because there is only one impl. @@ -559,7 +559,7 @@ public class DefsTest ColumnDefinition cdOld = meta.regularColumns().iterator().next(); ColumnDefinition cdNew = ColumnDefinition.regularDef(meta, cdOld.name.bytes, cdOld.type, null); meta.addOrReplaceColumnDefinition(cdNew); - MigrationManager.announceColumnFamilyUpdate(meta, false); + MigrationManager.announceColumnFamilyUpdate(meta); // check Assert.assertTrue(cfs.indexManager.getIndexes().isEmpty()); http://git-wip-us.apache.org/repos/asf/cassandra/blob/e5c40278/test/unit/org/apache/cassandra/triggers/TriggersSchemaTest.java ---------------------------------------------------------------------- diff --git a/test/unit/org/apache/cassandra/triggers/TriggersSchemaTest.java b/test/unit/org/apache/cassandra/triggers/TriggersSchemaTest.java index 577e7d3..58f743e 100644 --- a/test/unit/org/apache/cassandra/triggers/TriggersSchemaTest.java +++ b/test/unit/org/apache/cassandra/triggers/TriggersSchemaTest.java @@ -103,7 +103,7 @@ public class TriggersSchemaTest CFMetaData cfm2 = Schema.instance.getCFMetaData(ksName, cfName).copy(); TriggerDefinition td = TriggerDefinition.create(triggerName, triggerClass); cfm2.addTriggerDefinition(td); - MigrationManager.announceColumnFamilyUpdate(cfm2, false); + MigrationManager.announceColumnFamilyUpdate(cfm2); CFMetaData cfm3 = Schema.instance.getCFMetaData(ksName, cfName); assertFalse(cfm3.getTriggers().isEmpty()); @@ -126,7 +126,7 @@ public class TriggersSchemaTest CFMetaData cfm2 = Schema.instance.getCFMetaData(ksName, cfName).copy(); cfm2.removeTrigger(triggerName); - MigrationManager.announceColumnFamilyUpdate(cfm2, false); + MigrationManager.announceColumnFamilyUpdate(cfm2); CFMetaData cfm3 = Schema.instance.getCFMetaData(ksName, cfName).copy(); assertTrue(cfm3.getTriggers().isEmpty());
