CASSANDRA-21644 Fix concurrent SAI vector inserts failing once jvector's per-graph pool limit is exceeded - #5102
Conversation
…aph pool limit patch by Jeremiah Jordan; reviewed by XXX for CASSANDRA-21644
…ol limit is exceeded patch by Jeremiah Jordan; reviewed by XXX for CASSANDRA-21644
…urrent writers Review update: TrieMemtable and the locking ShardedSkipListMemtable serialize updates per shard, so when they have no more shards than jvector's per-builder limit there is nothing for the semaphore to bound. Add Memtable.limitsConcurrentWritesTo(int), false by default, and let OnHeapGraph leave the semaphore out when it is true. patch by Jeremiah Jordan; reviewed by XXX for CASSANDRA-21644
maedhroz
left a comment
maedhroz
left a comment
There was a problem hiding this comment.
Everything looks good, but the one gap I think we have now is that, because we use Mockito, VectorMemoryIndexTest actually never tests the non-semaphore happy path. Let's parameterize to have limitsConcurrentWritesTo() return an appropriate value based on how many threads we're actually firing?
| commit log span tracking) and `AbstractAllocatorMemtable` (adds memory management via the `Allocator` class, together | ||
| with flush triggering on memory use and time interval expiration). | ||
|
|
||
| A memtable that serializes writes, for example per shard, can say so through `limitsConcurrentWritesTo`. Secondary |
There was a problem hiding this comment.
| A memtable that serializes writes, for example per shard, can say so through `limitsConcurrentWritesTo`. Secondary | |
| A memtable that serializes writes, for example per shard, can say so through `limitsConcurrentWritesTo()`. Secondary |
nit
| public boolean limitsConcurrentWritesTo(int maxWriters) | ||
| { | ||
| return boundaries.shardCount() <= maxWriters; | ||
| } |
There was a problem hiding this comment.
We can't put this in AbstractShardedMemtable because of Locking?
There was a problem hiding this comment.
Right. The base ShardedSkipListMemtable does not lock, so is unbounded. Only the Locking one limits it, so can return true of the shard count is low enough.
| { | ||
| if (graphInsertPermits != null) | ||
| graphInsertPermits.release(1); | ||
| } |
There was a problem hiding this comment.
nit: The alternative to this null-checking stuff would be to have some kind of no-op Semaphore, but that does feel like overkill.
There was a problem hiding this comment.
yes. I think that would be overkill and I did not see a reason to replace the null check with a boolean check.
Review update: the mocked memtable always answered false to limitsConcurrentWritesTo(), so the tests never took the path where OnHeapGraph relies on the memtable and creates no semaphore. Have the mock answer according to the number of writer threads each test actually uses, so the tests within jvector's limit skip the semaphore and the test beyond it exercises it. The memtable implementations themselves are covered by MemtableConcurrentWriteLimitTest. Also the doc nit. patch by Jeremiah Jordan; reviewed by XXX for CASSANDRA-21644
9f290e2 to
ed31aa3
Compare
2 participants
Fix concurrent SAI vector inserts failing once jvector's per-graph pool limit is exceeded.