diff --git a/CHANGES.txt b/CHANGES.txt index 9377b7a421..9a49af3955 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,4 +1,5 @@ 5.0-alpha2 + * Correctly remove Index.Group from IndexRegistry (CASSANDRA-18905) * Fix vector type to support DDM's mask_default function (CASSANDRA-18889) * Remove unnecessary reporter-config3 dependency (CASSANDRA-18907) * Remove support for empty values on the vector data type (CASSANDRA-18876) diff --git a/src/java/org/apache/cassandra/db/lifecycle/Tracker.java b/src/java/org/apache/cassandra/db/lifecycle/Tracker.java index 061765bd52..e959c72fa9 100644 --- a/src/java/org/apache/cassandra/db/lifecycle/Tracker.java +++ b/src/java/org/apache/cassandra/db/lifecycle/Tracker.java @@ -86,7 +86,7 @@ public class Tracker { private static final Logger logger = LoggerFactory.getLogger(Tracker.class); - private final Collection subscribers = new CopyOnWriteArrayList<>(); + private final List subscribers = new CopyOnWriteArrayList<>(); public final ColumnFamilyStore cfstore; final AtomicReference view; @@ -560,6 +560,12 @@ public class Tracker subscribers.add(consumer); } + @VisibleForTesting + public boolean contains(INotificationConsumer consumer) + { + return subscribers.contains(consumer); + } + public void unsubscribe(INotificationConsumer consumer) { subscribers.remove(consumer); diff --git a/src/java/org/apache/cassandra/index/Index.java b/src/java/org/apache/cassandra/index/Index.java index f116fdb3e0..fc5f5a6f00 100644 --- a/src/java/org/apache/cassandra/index/Index.java +++ b/src/java/org/apache/cassandra/index/Index.java @@ -23,6 +23,7 @@ package org.apache.cassandra.index; import java.io.UncheckedIOException; import java.util.Collection; import java.util.Collections; +import java.util.Objects; import java.util.Optional; import java.util.Set; import java.util.concurrent.Callable; @@ -55,8 +56,8 @@ import org.apache.cassandra.index.transactions.IndexTransaction; import org.apache.cassandra.io.sstable.Component; import org.apache.cassandra.io.sstable.Descriptor; import org.apache.cassandra.io.sstable.ReducingKeyIterator; -import org.apache.cassandra.io.sstable.SSTableFlushObserver; import org.apache.cassandra.io.sstable.SSTable; +import org.apache.cassandra.io.sstable.SSTableFlushObserver; import org.apache.cassandra.io.sstable.format.SSTableReader; import org.apache.cassandra.schema.ColumnMetadata; import org.apache.cassandra.schema.IndexMetadata; @@ -275,6 +276,17 @@ public interface Index */ public void register(IndexRegistry registry); + /** + * Unregister current index when it's removed from system + * + * @param registry the index registry to register the instance with + */ + default void unregister(IndexRegistry registry) + { + // for singleton index, the group key is the index itself + registry.unregisterIndex(this, new Index.Group.Key(this)); + } + /** * If the index implementation uses a local table to store its index data, this method should return a * handle to it. If not, an empty {@link Optional} should be returned. This exists to support legacy @@ -677,11 +689,39 @@ public interface Index * Class providing grouped operations for indexes that communicate with each other. * * Index implementations should provide a {@code Group} implementation calling to - * {@link SecondaryIndexManager#registerIndex(Index, Object, Supplier)} during index registering + * {@link SecondaryIndexManager#registerIndex(Index, Index.Group.Key, Supplier)} during index registering * at {@link #register(IndexRegistry)} method. */ interface Group { + /** + * Group key is used to uniquely identify a {@link Group} within a table + */ + class Key + { + private final Object object; + + public Key(Object object) + { + this.object = object; + } + + @Override + public boolean equals(Object o) + { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + Key key = (Key) o; + return Objects.equals(object, key.object); + } + + @Override + public int hashCode() + { + return Objects.hash(object); + } + } + /** * Returns the indexes that are members of this group. * @@ -694,14 +734,16 @@ public interface Index * * @param index the index to be added */ - void addIndex(Index index); + default void addIndex(Index index) + {} /** * Removes the specified {@link Index} from the members of this group. * * @param index the index to be removed */ - void removeIndex(Index index); + default void removeIndex(Index index) + {} /** * Returns if this group contains the specified {@link Index}. @@ -711,6 +753,16 @@ public interface Index */ boolean containsIndex(Index index); + /** + * Returns whether this group can only ever contain a single index. + * + * @return {@code true} if this group only contains a single index, {@code false} otherwise + */ + default boolean isSingleton() + { + return true; + } + /** * Creates an new {@code Indexer} object for updates to a given partition. * @@ -769,8 +821,8 @@ public interface Index } /** - * Called when the table associated with this group has been invalidated. Implementations - * should dispose of any resources tied to the lifecycle of the {@link Group}. + * Called when the table associated with this group has been invalidated or all indexes in the group are removed. + * Implementations should dispose of any resources tied to the lifecycle of the {@link Group}. */ default void invalidate() { } diff --git a/src/java/org/apache/cassandra/index/IndexRegistry.java b/src/java/org/apache/cassandra/index/IndexRegistry.java index 308aeacd7a..46e87f357f 100644 --- a/src/java/org/apache/cassandra/index/IndexRegistry.java +++ b/src/java/org/apache/cassandra/index/IndexRegistry.java @@ -64,7 +64,12 @@ public interface IndexRegistry IndexRegistry EMPTY = new IndexRegistry() { @Override - public void registerIndex(Index index, Object groupKey, Supplier groupSupplier) + public void registerIndex(Index index, Index.Group.Key groupKey, Supplier groupSupplier) + { + } + + @Override + public void unregisterIndex(Index index, Index.Group.Key groupKey) { } @@ -125,7 +130,11 @@ public interface IndexRegistry public void register(IndexRegistry registry) { + } + @Override + public void unregister(IndexRegistry registry) + { } public Optional getBackingTable() @@ -245,7 +254,12 @@ public interface IndexRegistry } }; - public void registerIndex(Index index, Object groupKey, Supplier groupSupplier) + public void registerIndex(Index index, Index.Group.Key groupKey, Supplier groupSupplier) + { + } + + @Override + public void unregisterIndex(Index index, Index.Group.Key groupKey) { } @@ -277,9 +291,13 @@ public interface IndexRegistry default void registerIndex(Index index) { - registerIndex(index, index, () -> new SingletonIndexGroup(index)); + registerIndex(index, new Index.Group.Key(index), () -> new SingletonIndexGroup(index)); } - void registerIndex(Index index, Object groupKey, Supplier groupSupplier); + + void registerIndex(Index index, Index.Group.Key groupKey, Supplier groupSupplier); + + void unregisterIndex(Index index, Index.Group.Key groupKey); + Collection listIndexGroups(); Index getIndex(IndexMetadata indexMetadata); diff --git a/src/java/org/apache/cassandra/index/SecondaryIndexManager.java b/src/java/org/apache/cassandra/index/SecondaryIndexManager.java index 19985c304f..9fd24bcec3 100644 --- a/src/java/org/apache/cassandra/index/SecondaryIndexManager.java +++ b/src/java/org/apache/cassandra/index/SecondaryIndexManager.java @@ -162,7 +162,7 @@ public class SecondaryIndexManager implements IndexRegistry, INotificationConsum /** * The groups of all the registered indexes */ - private final Map indexGroups = Maps.newConcurrentMap(); + private final Map indexGroups = Maps.newConcurrentMap(); /** * The count of pending index builds for each index. @@ -344,15 +344,17 @@ public class SecondaryIndexManager implements IndexRegistry, INotificationConsum public synchronized void removeIndex(String indexName) { - Index index = unregisterIndex(indexName); - if (null != index) + Index removedIndex = indexes.remove(indexName); + + if (removedIndex != null) { + removedIndex.unregister(this); + markIndexRemoved(indexName); - executeBlocking(index.getInvalidateTask(), null); + executeBlocking(removedIndex.getInvalidateTask(), null); } } - public Set getDependentIndexes(ColumnMetadata column) { if (indexes.isEmpty()) @@ -1302,7 +1304,8 @@ public class SecondaryIndexManager implements IndexRegistry, INotificationConsum /* * IndexRegistry methods */ - public void registerIndex(Index index, Object groupKey, Supplier groupSupplier) + @Override + public void registerIndex(Index index, Index.Group.Key groupKey, Supplier groupSupplier) { String name = index.getIndexMetadata().name; indexes.put(name, index); @@ -1312,41 +1315,26 @@ public class SecondaryIndexManager implements IndexRegistry, INotificationConsum Index.Group group = indexGroups.computeIfAbsent(groupKey, k -> groupSupplier.get()); // add the created index to its group if it is not a singleton group - if (!(group instanceof SingletonIndexGroup)) - { - if (index.getBackingTable().isPresent()) - throw new InvalidRequestException("Indexes belonging to a group of indexes shouldn't have a backing table"); - - group.addIndex(index); - } + group.addIndex(index); } - private Index unregisterIndex(String name) + @Override + public void unregisterIndex(Index removed, Index.Group.Key groupKey) { - Index removed = indexes.remove(name); - logger.trace(removed == null ? "Index {} was not registered" : "Removed index {} from registry", name); - - if (removed != null) + Index.Group group = indexGroups.get(groupKey); + if (group != null && group.containsIndex(removed)) { - // Remove the index from any non-singleton groups... - for (Index.Group group : listIndexGroups()) + // Remove the index from non-singleton groups... + group.removeIndex(removed); + + // if the group is a singleton or there are no more indexes left in the group, remove it + if (group.isSingleton() || group.getIndexes().isEmpty()) { - if (!(group instanceof SingletonIndexGroup) && group.containsIndex(removed)) - { - group.removeIndex(removed); - - if (group.getIndexes().isEmpty()) - { - indexGroups.remove(group); - } - } + Index.Group removedGroup = indexGroups.remove(groupKey); + if (removedGroup != null) + removedGroup.invalidate(); } - - // ...and remove singleton groups entirely. - indexGroups.remove(removed); } - - return removed; } public Index getIndex(IndexMetadata metadata) @@ -1364,14 +1352,14 @@ public class SecondaryIndexManager implements IndexRegistry, INotificationConsum return ImmutableSet.copyOf(indexGroups.values()); } - public Index.Group getIndexGroup(Object key) + public Index.Group getIndexGroup(Index.Group.Key key) { return indexGroups.get(key); } /** * Returns the {@link Index.Group} the specified index belongs to, as specified during registering with - * {@link #registerIndex(Index, Object, Supplier)}. + * {@link #registerIndex(Index, Index.Group.Key, Supplier)}. * * @param metadata the index metadata * @return the group the index belongs to, or {@code null} if the index is not registered or if it hasn't been diff --git a/src/java/org/apache/cassandra/index/SingletonIndexGroup.java b/src/java/org/apache/cassandra/index/SingletonIndexGroup.java index 304d35d6d4..162247fd74 100644 --- a/src/java/org/apache/cassandra/index/SingletonIndexGroup.java +++ b/src/java/org/apache/cassandra/index/SingletonIndexGroup.java @@ -62,18 +62,6 @@ public class SingletonIndexGroup implements Index.Group return delegate; } - @Override - public void addIndex(Index index) - { - throw new UnsupportedOperationException(); - } - - @Override - public void removeIndex(Index index) - { - throw new UnsupportedOperationException(); - } - @Override public boolean containsIndex(Index index) { @@ -89,9 +77,8 @@ public class SingletonIndexGroup implements Index.Group IndexTransaction.Type transactionType, Memtable memtable) { - return indexSelector.test(delegate) - ? delegate.indexerFor(key, columns, nowInSec, ctx, transactionType, memtable) - : null; + return indexSelector.test(delegate) ? delegate.indexerFor(key, columns, nowInSec, ctx, transactionType, memtable) + : null; } @Override diff --git a/src/java/org/apache/cassandra/index/sai/StorageAttachedIndex.java b/src/java/org/apache/cassandra/index/sai/StorageAttachedIndex.java index a0cdf9c3ac..b09f04d80d 100644 --- a/src/java/org/apache/cassandra/index/sai/StorageAttachedIndex.java +++ b/src/java/org/apache/cassandra/index/sai/StorageAttachedIndex.java @@ -272,7 +272,13 @@ public class StorageAttachedIndex implements Index public void register(IndexRegistry registry) { // index will be available for writes - registry.registerIndex(this, StorageAttachedIndexGroup.class, () -> new StorageAttachedIndexGroup(baseCfs)); + registry.registerIndex(this, StorageAttachedIndexGroup.GROUP_KEY, () -> new StorageAttachedIndexGroup(baseCfs)); + } + + @Override + public void unregister(IndexRegistry registry) + { + registry.unregisterIndex(this, StorageAttachedIndexGroup.GROUP_KEY); } @Override diff --git a/src/java/org/apache/cassandra/index/sai/StorageAttachedIndexGroup.java b/src/java/org/apache/cassandra/index/sai/StorageAttachedIndexGroup.java index de4c8b3570..5dac6edd47 100644 --- a/src/java/org/apache/cassandra/index/sai/StorageAttachedIndexGroup.java +++ b/src/java/org/apache/cassandra/index/sai/StorageAttachedIndexGroup.java @@ -22,6 +22,7 @@ import java.util.Collections; import java.util.HashSet; import java.util.Objects; import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; import java.util.function.Predicate; import java.util.stream.Collectors; import javax.annotation.Nullable; @@ -29,7 +30,6 @@ import javax.annotation.concurrent.ThreadSafe; import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableSet; -import com.google.common.collect.Sets; import com.google.common.primitives.Ints; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -75,10 +75,12 @@ public class StorageAttachedIndexGroup implements Index.Group, INotificationCons { private static final Logger logger = LoggerFactory.getLogger(StorageAttachedIndexGroup.class); + public static final Index.Group.Key GROUP_KEY = new Index.Group.Key(StorageAttachedIndexGroup.class); + private final TableQueryMetrics queryMetrics; private final TableStateMetrics stateMetrics; private final IndexGroupMetrics groupMetrics; - private final Set indexes = Sets.newConcurrentHashSet(); + private final Set indexes = ConcurrentHashMap.newKeySet(); private final ColumnFamilyStore baseCfs; private final SSTableContextManager contextManager; @@ -98,7 +100,7 @@ public class StorageAttachedIndexGroup implements Index.Group, INotificationCons @Nullable public static StorageAttachedIndexGroup getIndexGroup(ColumnFamilyStore cfs) { - return (StorageAttachedIndexGroup) cfs.indexManager.getIndexGroup(StorageAttachedIndexGroup.class); + return (StorageAttachedIndexGroup) cfs.indexManager.getIndexGroup(StorageAttachedIndexGroup.GROUP_KEY); } @Override @@ -128,14 +130,13 @@ public class StorageAttachedIndexGroup implements Index.Group, INotificationCons for (SSTableReader sstable : contextManager.sstables()) sstable.unregisterComponents(IndexDescriptor.create(sstable).getLivePerSSTableComponents(), baseCfs.getTracker()); deletePerSSTableFiles(baseCfs.getLiveSSTables()); - baseCfs.getTracker().unsubscribe(this); } } @Override public void invalidate() { - // in case of dropping table, sstable contexts should already been removed by SSTableListChangedNotification. + // in case of removing last index from group, sstable contexts should already been removed by removeIndex queryMetrics.release(); groupMetrics.release(); stateMetrics.release(); @@ -149,6 +150,12 @@ public class StorageAttachedIndexGroup implements Index.Group, INotificationCons return indexes.contains(index); } + @Override + public boolean isSingleton() + { + return false; + } + @Override public Index.Indexer indexerFor(Predicate indexSelector, DecoratedKey key, diff --git a/src/java/org/apache/cassandra/index/sasi/SASIIndex.java b/src/java/org/apache/cassandra/index/sasi/SASIIndex.java index 1ae3a04943..93448f9e78 100644 --- a/src/java/org/apache/cassandra/index/sasi/SASIIndex.java +++ b/src/java/org/apache/cassandra/index/sasi/SASIIndex.java @@ -186,7 +186,7 @@ public class SASIIndex implements Index, INotificationConsumer @Override public void register(IndexRegistry registry) { - registry.registerIndex(this, this, () -> new SASIIndexGroup(this)); + registry.registerIndex(this, new Group.Key(this), () -> new SASIIndexGroup(this)); } public IndexMetadata getIndexMetadata() diff --git a/test/unit/org/apache/cassandra/index/CustomIndexTest.java b/test/unit/org/apache/cassandra/index/CustomIndexTest.java index 5c5f89f644..79987f34b1 100644 --- a/test/unit/org/apache/cassandra/index/CustomIndexTest.java +++ b/test/unit/org/apache/cassandra/index/CustomIndexTest.java @@ -25,6 +25,7 @@ import java.util.concurrent.Callable; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; import java.util.function.Predicate; +import java.util.function.Supplier; import java.util.stream.Collectors; import com.google.common.collect.ImmutableList; @@ -1399,13 +1400,13 @@ public class CustomIndexTest extends CQLTester // create two indexes belonging to the same group and verify that only one group is added to the manager String idx1 = createIndex(String.format("CREATE CUSTOM INDEX ON %%s(v1) USING '%s'", indexClassName)); String idx2 = createIndex(String.format("CREATE CUSTOM INDEX ON %%s(v2) USING '%s'", indexClassName)); - IndexWithSharedGroup.Group group = indexManager.listIndexGroups() - .stream() - .filter(g -> g instanceof IndexWithSharedGroup.Group) - .map(g -> (IndexWithSharedGroup.Group) g) - .findAny() - .orElseThrow(AssertionError::new); - + Supplier groupSupplier = + () -> indexManager.listIndexGroups().stream() + .filter(g -> g instanceof IndexWithSharedGroup.Group) + .map(g -> (IndexWithSharedGroup.Group) g) + .findAny() + .orElse(null); + IndexWithSharedGroup.Group group = groupSupplier.get(); // verify that only one group has been added to the manager assertEquals(2, indexManager.listIndexes().size()); assertEquals(1, indexManager.listIndexGroups().size()); @@ -1435,20 +1436,26 @@ public class CustomIndexTest extends CQLTester assertEquals(2, indexManager.listIndexes().size()); assertEquals(1, indexManager.listIndexGroups().size()); - // drop the remaining members of the shared group and verify that it is kept empty in the manager + // drop the remaining members of the shared group and verify that it no longer exists in the manager dropIndex("DROP INDEX %s." + idx2); dropIndex("DROP INDEX %s." + idx5); assertEquals(0, indexManager.listIndexes().size()); - assertEquals(1, indexManager.listIndexGroups().size()); + assertEquals(0, indexManager.listIndexGroups().size()); assertEquals(0, group.indexes.size()); - // create the sharing group members again and verify that they are added to the existing group instance + // create the sharing group members again and verify that they are added to a new group instance createIndex(String.format("CREATE CUSTOM INDEX %s ON %%s(v1) USING '%s'", idx1, indexClassName)); createIndex(String.format("CREATE CUSTOM INDEX %s ON %%s(v2) USING '%s'", idx2, indexClassName)); createIndex(String.format("CREATE CUSTOM INDEX %s ON %%s(v3) USING '%s'", idx3, indexClassName)); + IndexWithSharedGroup.Group newGroup = indexManager.listIndexGroups() + .stream() + .filter(g -> g instanceof IndexWithSharedGroup.Group) + .map(g -> (IndexWithSharedGroup.Group) g) + .findAny() + .orElseThrow(AssertionError::new); assertEquals(3, indexManager.listIndexes().size()); assertEquals(1, indexManager.listIndexGroups().size()); - assertEquals(3, group.indexes.size()); + assertEquals(3, newGroup.indexes.size()); } /** @@ -1471,7 +1478,13 @@ public class CustomIndexTest extends CQLTester @Override public void register(IndexRegistry registry) { - registry.registerIndex(this, Group.class, Group::new); + registry.registerIndex(this, new Group.Key(Group.class), Group::new); + } + + @Override + public void unregister(IndexRegistry registry) + { + registry.unregisterIndex(this, new Group.Key(Group.class)); } private static class Group implements Index.Group @@ -1533,6 +1546,12 @@ public class CustomIndexTest extends CQLTester return indexes.containsKey(index.getIndexMetadata().name); } + @Override + public boolean isSingleton() + { + return false; + } + @Override public Index.Indexer indexerFor(Predicate indexSelector, DecoratedKey key, @@ -1663,36 +1682,4 @@ public class CustomIndexTest extends CQLTester } } } - - @Test - public void testMulticolumnIndexWithBaseTable() throws Throwable - { - createTable("CREATE TABLE %s(k int PRIMARY KEY, v int)"); - assertInvalidMessage("Indexes belonging to a group of indexes shouldn't have a backing table", - String.format("CREATE CUSTOM INDEX ON %%s(v) USING '%s'", - MulticolumnIndexWithBaseTable.class.getName())); - } - - public static final class MulticolumnIndexWithBaseTable extends StubIndex - { - private final ColumnFamilyStore baseCfs; - - public MulticolumnIndexWithBaseTable(ColumnFamilyStore baseCfs, IndexMetadata metadata) - { - super(baseCfs, metadata); - this.baseCfs = baseCfs; - } - - @Override - public void register(IndexRegistry registry) - { - registry.registerIndex(this, MulticolumnIndexWithBaseTable.class, StubIndexGroup::new); - } - - @Override - public Optional getBackingTable() - { - return Optional.of(baseCfs); - } - } } diff --git a/test/unit/org/apache/cassandra/index/StubIndexGroup.java b/test/unit/org/apache/cassandra/index/StubIndexGroup.java index 0b6ad1c37d..22dfbe262b 100644 --- a/test/unit/org/apache/cassandra/index/StubIndexGroup.java +++ b/test/unit/org/apache/cassandra/index/StubIndexGroup.java @@ -67,6 +67,12 @@ public class StubIndexGroup implements Index.Group return indexes.contains(index); } + @Override + public boolean isSingleton() + { + return false; + } + @Override public Index.Indexer indexerFor(Predicate indexSelector, DecoratedKey key, diff --git a/test/unit/org/apache/cassandra/index/sai/cql/IndexGroupLifecycleTest.java b/test/unit/org/apache/cassandra/index/sai/cql/IndexGroupLifecycleTest.java new file mode 100644 index 0000000000..13965439ab --- /dev/null +++ b/test/unit/org/apache/cassandra/index/sai/cql/IndexGroupLifecycleTest.java @@ -0,0 +1,81 @@ +/* + * 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 + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.cassandra.index.sai.cql; + +import org.junit.Test; + +import org.apache.cassandra.db.ColumnFamilyStore; +import org.apache.cassandra.db.lifecycle.Tracker; +import org.apache.cassandra.index.sai.SAITester; +import org.apache.cassandra.index.sai.StorageAttachedIndexGroup; + +import static java.lang.String.format; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotSame; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; + +public class IndexGroupLifecycleTest extends SAITester +{ + @Test + public void testDropAndRecreate() throws Throwable + { + createTable("CREATE TABLE %s (pk text, value text, PRIMARY KEY (pk))"); + populateOneSSTable(); + + ColumnFamilyStore cfs = getCurrentColumnFamilyStore(); + cfs.disableAutoCompaction(); + Tracker tracker = cfs.getTracker(); + + // create index and drop it: StorageAttachedIndexGroup should be removed + createIndex("CREATE CUSTOM INDEX sai ON %s(value) USING 'StorageAttachedIndex'"); + + StorageAttachedIndexGroup group = (StorageAttachedIndexGroup) cfs.indexManager.getIndexGroup(StorageAttachedIndexGroup.GROUP_KEY); + assertTrue(tracker.contains(group)); + assertEquals(1, group.sstableContextManager().size()); + + dropIndex(format("DROP INDEX %s.sai", KEYSPACE)); + assertFalse(tracker.contains(group)); + assertEquals(0, group.sstableContextManager().size()); // sstable should be cleared from old group + assertNull(cfs.indexManager.getIndexGroup(StorageAttachedIndexGroup.GROUP_KEY)); + + // populate 2nd sstable. Old group should not track it + populateOneSSTable(); + assertEquals(0, group.sstableContextManager().size()); + + // create index again: expect a new StorageAttachedIndexGroup to be registered into tracker + createIndex("CREATE CUSTOM INDEX sai ON %s(value) USING 'StorageAttachedIndex'"); + + StorageAttachedIndexGroup newGroup = (StorageAttachedIndexGroup) cfs.indexManager.getIndexGroup(StorageAttachedIndexGroup.GROUP_KEY); + assertNotSame(group, newGroup); + assertTrue(tracker.contains(newGroup)); + assertEquals(2, newGroup.sstableContextManager().size()); + + // populate 3rd sstable. new group should track it + populateOneSSTable(); + assertEquals(3, newGroup.sstableContextManager().size()); + } + + private void populateOneSSTable() + { + execute("INSERT INTO %s(pk, value) VALUES('k', 'v')"); + flush(); + } +} \ No newline at end of file diff --git a/test/unit/org/apache/cassandra/index/sai/cql/StorageAttachedIndexDDLTest.java b/test/unit/org/apache/cassandra/index/sai/cql/StorageAttachedIndexDDLTest.java index 6dedd5ed2d..f10149855d 100644 --- a/test/unit/org/apache/cassandra/index/sai/cql/StorageAttachedIndexDDLTest.java +++ b/test/unit/org/apache/cassandra/index/sai/cql/StorageAttachedIndexDDLTest.java @@ -84,6 +84,7 @@ import static java.util.Collections.singletonList; import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import static org.mockito.Mockito.when; @@ -667,8 +668,7 @@ public class StorageAttachedIndexDDLTest extends SAITester dropIndex("DROP INDEX %s." + literalIndexName); verifyIndexFiles(numericIndexContext, literalIndexContext, 0, 0); - verifySSTableIndexes(numericIndexName, 0); - verifySSTableIndexes(literalIndexName, 0); + assertNull(getCurrentIndexGroup()); assertEquals("Segment memory limiter should revert to zero on drop.", 0L, getSegmentBufferUsedBytes()); assertEquals("There should be no segment builders in progress.", 0L, getColumnIndexBuildsInProgress()); @@ -1182,7 +1182,7 @@ public class StorageAttachedIndexDDLTest extends SAITester delayIndexBuilderCompletion.disable(); - verifySSTableIndexes(indexName, 0); + assertNull(getCurrentIndexGroup()); assertFalse("Expect index not built", SystemKeyspace.isIndexBuilt(KEYSPACE, indexName)); // create index again, it should succeed diff --git a/test/unit/org/apache/cassandra/index/sai/functional/CompactionTest.java b/test/unit/org/apache/cassandra/index/sai/functional/CompactionTest.java index da9a051f4f..aff3c5e862 100644 --- a/test/unit/org/apache/cassandra/index/sai/functional/CompactionTest.java +++ b/test/unit/org/apache/cassandra/index/sai/functional/CompactionTest.java @@ -61,8 +61,8 @@ import org.apache.cassandra.utils.TimeUUID; import org.apache.cassandra.utils.concurrent.Refs; import static org.assertj.core.api.Assertions.assertThatThrownBy; -import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; @@ -339,8 +339,7 @@ public class CompactionTest extends SAITester } // verify index group metrics are cleared. - assertEquals(0, getOpenIndexFiles()); - assertEquals(0, getDiskUsage()); + assertNull(getCurrentIndexGroup()); // verify indexes are dropped // verify indexes are dropped diff --git a/test/unit/org/apache/cassandra/index/sai/metrics/IndexGroupMetricsTest.java b/test/unit/org/apache/cassandra/index/sai/metrics/IndexGroupMetricsTest.java index 7a54ebfc86..26a7ab6714 100644 --- a/test/unit/org/apache/cassandra/index/sai/metrics/IndexGroupMetricsTest.java +++ b/test/unit/org/apache/cassandra/index/sai/metrics/IndexGroupMetricsTest.java @@ -26,6 +26,7 @@ import org.apache.cassandra.index.sai.disk.format.Version; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertNull; public class IndexGroupMetricsTest extends AbstractMetricsTest { @@ -76,8 +77,7 @@ public class IndexGroupMetricsTest extends AbstractMetricsTest // drop last index, no open index files dropIndex("DROP INDEX %s." + v1IndexName); - assertEquals(0, getOpenIndexFiles()); - assertEquals(0, getDiskUsage()); + assertNull(getCurrentIndexGroup()); } protected int getOpenIndexFiles() diff --git a/test/unit/org/apache/cassandra/index/sai/metrics/QueryMetricsTest.java b/test/unit/org/apache/cassandra/index/sai/metrics/QueryMetricsTest.java index 14931b173b..e1f1c6933d 100644 --- a/test/unit/org/apache/cassandra/index/sai/metrics/QueryMetricsTest.java +++ b/test/unit/org/apache/cassandra/index/sai/metrics/QueryMetricsTest.java @@ -91,12 +91,8 @@ public class QueryMetricsTest extends AbstractMetricsTest assertEquals(1L, getTableQueryMetrics(keyspace, table, "TotalQueriesCompleted")); - // Even if we drop the last index on the table, table-level metrics should still be visible: + // If we drop the last index on the table we should no longer see the table-level state metrics: dropIndex(String.format("DROP INDEX %s." + index, keyspace)); - assertEquals(1L, getTableQueryMetrics(keyspace, table, "TotalQueriesCompleted")); - - // When the whole table is dropped, we should finally fail to find table-level metrics: - dropTable(String.format("DROP TABLE %s." + table, keyspace)); assertThatThrownBy(() -> getTableQueryMetrics(keyspace, table, "TotalQueriesCompleted")).hasCauseInstanceOf(InstanceNotFoundException.class); } diff --git a/test/unit/org/apache/cassandra/index/sai/metrics/StateMetricsTest.java b/test/unit/org/apache/cassandra/index/sai/metrics/StateMetricsTest.java index e6ecda02fb..8e83293e61 100644 --- a/test/unit/org/apache/cassandra/index/sai/metrics/StateMetricsTest.java +++ b/test/unit/org/apache/cassandra/index/sai/metrics/StateMetricsTest.java @@ -55,12 +55,8 @@ public class StateMetricsTest extends AbstractMetricsTest assertEquals(1, rows.all().size()); assertEquals(1L, getTableStateMetrics(keyspace, table, "TotalIndexCount")); - // If we drop the last index on the table, table-level state metrics should still be visible: + // If we drop the last index on the table, we should no longer see the table-level state metrics: dropIndex(String.format("DROP INDEX %s." + index, keyspace)); - assertEquals(0L, getTableStateMetrics(keyspace, table, "TotalIndexCount")); - - // When the whole table is dropped, we should finally fail to find table-level state metrics: - dropTable(String.format("DROP TABLE %s." + table, keyspace)); assertThatThrownBy(() -> getTableStateMetrics(keyspace, table, "TotalIndexCount")).hasCauseInstanceOf(InstanceNotFoundException.class); }