maedhroz commented on code in PR #2769:
URL: https://github.com/apache/cassandra/pull/2769#discussion_r1360910042
##########
src/java/org/apache/cassandra/index/sai/StorageAttachedIndexGroup.java:
##########
@@ -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<StorageAttachedIndex> indexes =
Sets.newConcurrentHashSet();
+ private final Set<StorageAttachedIndex> indexes =
ConcurrentHashMap.newKeySet();
Review Comment:
This is safe because `SIM#createIndex()` is `synchronized`?
##########
src/java/org/apache/cassandra/index/sai/StorageAttachedIndexGroup.java:
##########
@@ -128,14 +130,13 @@ public void removeIndex(Index index)
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
Review Comment:
...and per-column index files are removed via `SIM#reload()`, correct?
##########
src/java/org/apache/cassandra/index/SecondaryIndexManager.java:
##########
@@ -1312,41 +1315,25 @@ public void registerIndex(Index index, Object groupKey,
Supplier<Index.Group> gr
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)
+ public void unregisterIndex(Index removed, Index.Group.Key groupKey)
Review Comment:
nit: Throw `@Override` on there I guess?
##########
src/java/org/apache/cassandra/index/SingletonIndexGroup.java:
##########
@@ -42,13 +44,11 @@
*/
public class SingletonIndexGroup implements Index.Group
{
- private final Index delegate;
- private final Set<Index> indexes;
+ private volatile Index delegate;
Review Comment:
I've been mulling over the change here around `delegate`, `registerIndex()`,
and `unregisterIndex()` (in `SIM`)...
I'd propose something like this:
1.) Revert all the changes in the singleton group here, and return to having
the index passed to it at construction.
2.) Just have `addIndex()` and `removeIndex()` no-op, or even inherit
default implementations from `Index`. (no-op might be useful even if just to
have a comment there that explains why we no-op)
3.) In `SIM#unregisterIndex()`, take advantage of the fact that if the group
contains the index to remove and only has 1 index anyway, we can remove the
group without special-casing the logic for the singleton case. (There are other
options, like having an `isSingleton()` method on `Group` that defaults to
returning `true`.)
The benefits would be:
1.) The singleton group gets less complex again, and is trivially
thread-safe again.
2.) I know we probably aren't worried too much about this, but any minor
performance penalty for volatile access goes away again.
3.) Some existing comments in `SIM` become correct enough again that we
don't have to alter them. (ex. "Remove the index from non-singleton groups...")
--
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]
