maedhroz commented on code in PR #4196:
URL: https://github.com/apache/cassandra/pull/4196#discussion_r2141019018
##########
src/java/org/apache/cassandra/index/sai/memory/MemtableIndexManager.java:
##########
@@ -134,47 +127,15 @@ public MemtableIndex
getPendingMemtableIndex(LifecycleNewTracker tracker)
.orElse(null);
}
- public KeyRangeIterator searchMemtableIndexes(QueryContext queryContext,
Expression e, AbstractBounds<PartitionPosition> keyRange)
- {
- Collection<MemtableIndex> memtableIndexes =
liveMemtableIndexMap.values();
-
- if (memtableIndexes.isEmpty())
- {
- return KeyRangeIterator.empty();
- }
-
- KeyRangeIterator.Builder builder =
KeyRangeUnionIterator.builder(memtableIndexes.size());
-
- for (MemtableIndex memtableIndex : memtableIndexes)
- {
- builder.add(memtableIndex.search(queryContext, e, keyRange));
- }
-
- return builder.build();
- }
-
- public KeyRangeIterator limitToTopResults(QueryContext context,
List<PrimaryKey> source, Expression e)
+ public long liveMemtableWriteCount()
{
- Collection<MemtableIndex> memtables = liveMemtableIndexMap.values();
-
- if (memtables.isEmpty())
- {
- return KeyRangeIterator.empty();
- }
-
- KeyRangeUnionIterator.Builder builder =
KeyRangeUnionIterator.builder(memtables.size());
-
- for (MemtableIndex index : memtables)
- {
- builder.add(index.limitToTopResults(source, e,
context.vectorContext().limit()));
- }
-
- return builder.build();
+ return
liveMemtableIndexMap.values().stream().mapToLong(MemtableIndex::writeCount).sum();
}
- public long liveMemtableWriteCount()
+ public Collection<MemtableIndex> getLiveMemtableIndexes()
{
- return
liveMemtableIndexMap.values().stream().mapToLong(MemtableIndex::writeCount).sum();
+ // Copy the values. Otherwise, we'll only have a view of the map's
values which is subject to change.
+ return new ArrayList<>(liveMemtableIndexMap.values());
Review Comment:
There's one small optimization we've eliminated here, i.e. when the values
collection is empty. Should we preserve that path? Something like...
```
Collection<MemtableIndex> memtableIndexes = liveMemtableIndexMap.values();
if (memtableIndexes.isEmpty())
return Collections.emptySet();
// Copy the values. Otherwise, we'll only have a view of the map's values
which is subject to change.
return new ArrayList<>(memtableIndexes);
```
The defensive copy seems critical to the correctness of the fix, and there
aren't other callers that want to avoid it. I wonder if
`getLiveIndexesSnapshot()` or something similar would help on the caller
readability side, but feel free to ignore that (really).
--
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: pr-unsubscr...@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscr...@cassandra.apache.org
For additional commands, e-mail: pr-h...@cassandra.apache.org
