[
https://issues.apache.org/jira/browse/CASSANDRA-18216?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18090749#comment-18090749
]
Caleb Rackliffe commented on CASSANDRA-18216:
---------------------------------------------
One other thing...posting the one finding turned up by our Claude review skill:
{noformat}
Findings (ranked by confidence)
Finding 1: Missing DDL-time validation of the shards option
- Location: src/java/org/apache/cassandra/index/sai/StorageAttachedIndex.java
validateOptions(...) (the new block reading shardsOption)
- Confidence: High
- Flagged by: Absence (primary), Resources (corroborating)
- What's wrong: validateOptions only rejects shards on vector indexes. It
does NOT parse the value as an integer, and does NOT range-check it. Parsing is
deferred to first write via Integer.parseInt(shardsOption) in
MemtableIndexManager.initializeMemtableIndex (MemtableIndexManager.java:71).
- Consequences:
- CREATE INDEX ... WITH OPTIONS = {'shards':'abc'} succeeds at DDL time,
then every subsequent write to the table throws NumberFormatException from
inside the write path.
- shards = '0', '-5', or '1' silently falls through to
UnshardedMemtableIndex because the only check is if (shardCount > 1) — the user
gets no signal their option had no effect.
- Positive evidence: Every other integer-valued SAI option follows the
validate-at-DDL pattern. IndexWriterConfig.fromOptions (called from
StorageAttachedIndex.validateOptions:286) wraps
Integer.parseInt(MAXIMUM_NODE_CONNECTIONS) and CONSTRUCTION_BEAM_WIDTH in
try/catch + throws InvalidRequestException, then range-checks
(IndexWriterConfig.java:125-152).
- Suggested fix: In validateOptions, after the vector check, wrap
Integer.parseInt(shardsOption) in try/catch and throw InvalidRequestException
on NumberFormatException or non-positive values, mirroring
IndexWriterConfig.fromOptions.
Specialist Coverage
- Logic: no High findings
- Boundary: no High findings
- Concurrency: no High findings
- Resources: no High findings (corroborated Finding 1 as Medium)
- Absence: 1 High finding (Finding 1)
- Completeness: no High findings
- Symmetry: no High asymmetries
Convergent Medium-confidence theme (below the High bar but worth noting)
Four specialists (Resources, Logic, Boundary, Symmetry) independently flagged
the new Range.intersects(ExcludingBounds) and
Range.intersects(IncludingExcludingBounds) overloads as having suspicious
wrap-around semantics — specifically the early-return on
this.left.compareTo(that.left) == 0 (and this.right.compareTo(that.right) == 0)
when this is a wrap-around range. Each specialist dropped it from High
because:
1. The only new caller in this patch (ShardBoundaries.precomputeRanges →
getShardsForRange) builds non-wrapping ranges, so the buggy branch isn't
reachable through the patch's own code.
2. The patch's tests in RangeIntersectsBoundsTest encode the current behavior
as expected (e.g. assertFalse(someWrapped.intersects(bl)) where someWrapped =
(8, 4] and bl = [3, 4) — these share token 3, so the assertion appears
semantically wrong but is asserted as the intended behavior).
However, the patch turns these overloads from UnsupportedOperationException
into returning answers, which silently changes behavior for existing callers
(DiagnosticSnapshotService, CompactionManager were noted), so this is worth a
manual look even though it doesn't meet your strict High bar. I'd recommend
either (a) confirming the
wrap-around semantics encoded in the tests are intentional and documenting
why, or (b) opening this as a follow-up to revisit before any future caller
relies on these overloads with wrap-around inputs.
{noformat}
We can talk about this here or offline.
> Allow sharding of the SAI in-memory index
> ------------------------------------------
>
> Key: CASSANDRA-18216
> URL: https://issues.apache.org/jira/browse/CASSANDRA-18216
> Project: Apache Cassandra
> Issue Type: Improvement
> Components: Feature/SAI
> Reporter: Mike Adamson
> Assignee: Yash Sunil Nasery
> Priority: Normal
> Labels: SAI
> Fix For: 6.x
>
>
> The Memtable implementation allows it to be split into a number of shards.
> This reduces contention during writes.
> The MemtableIndex in SAI should follow this practice. It currently does not
> support sharding so all writes hit the same synchronized write block. The
> in-memory index search can also use the sharding to reduce the number of
> indexes that it searches based on the key range passed to the search.
> This exists already in the DS fork:
> https://github.com/datastax/cassandra/pull/298
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
