Serve Cassandra cursor repositioning with CQL slice queries instead of a no-op iterator restart

[vharseko]

Follow-up to the JDBC cursor analysis in #863/#860: the Cassandra backend's CursorImpl has the same class of problem, but worse — broken repositioning semantics, not just excess traffic.

Problem

The DataStax driver's ResultSet is consumed once: rc.iterator() always returns the same iterator (MultiPageResultSet returns the same RowIterator field; SinglePageResultSet.currentPage() is () -> iterator polling rows off a queue), so the //restart iterator lines never rewound anything. Consequences:

• positionToKeyOrNext(k) backward returned the next remaining row (silently skipping [k .. current]), or false after exhaustion even though matching rows exist; repositioning to the current key moved the cursor forward instead of staying.
• positionToKey(k) backward drained the whole remaining partition over the network and returned false for a key that exists. DN2URI.targetEntryReferrals walks up the DIT, so every one of its lookups is a backward one: referrals above the target were never found.
• positionToIndex(i) counted from the current position, not from the first row — VLVIndex.evaluateVLVRequestByAssertion does positionToKeyOrNext(assertion) && positionToIndex(0) on one cursor, so VLV searches with an assertion returned wrong offsets.
• No seek and no page sizing at all: the only query was a full-partition SELECT ... ORDER BY key at the driver default of 5000 rows per page, so positioning to a key pulled every row before it, every openCursor eagerly fetched the first page of the whole partition, and a point lookup such as ID2Entry.containsEntryID paid for thousands of full entries.

Changes

Repositioning

• Repositioning that cannot be served by moving forward runs a server-side slice on the clustering column: and key>=:key ORDER BY key (the table is PRIMARY KEY ((baseDN,indexId),key), blob clustering collates in unsigned byte order — same property the JDBC fix relies on).
• Forward repositioning within the rows the driver already fetched is served without a query, for both positionToKeyOrNext and positionToKey (one shared forwardInPage walk). The ascending positionToKey loops on a single shared id2entry cursor in EntryContainer.deleteSubtree and renameSubtree therefore stay at one forward pass instead of one slice per entry.
• positionToIndex restarts from the first row (CQL has no offset clause) and asks for the rows it has to walk in one page; positionToLastKey is ORDER BY key DESC LIMIT 1 instead of draining the partition.
• Cursors are lazy: opening one no longer starts a full partition scan.

Paging

• A cursor sizes its own pages instead of inheriting the driver default of 5000 rows: it starts at 32 rows and doubles up to 1000 while a scan keeps outrunning it. Both bounds are tunable with org.openidentityplatform.opendj.cassandra.fetchsize.initial and org.openidentityplatform.opendj.cassandra.fetchsize, mirroring org.openidentityplatform.opendj.jdbc.fetchsize.
• A scan that runs past the end of its page continues with a keyset slice (and key>:key ORDER BY key) at the bigger page size, rather than letting the driver page at the size the cursor started with.

Semantics that used to be undefined

• A missed positionToKey leaves the cursor undefined and stopped just before the first key after the missing one, so a following next() returns that row instead of skipping it (this is what pdb does).
• A closed cursor is undefined and every navigation on it returns false, like EmptyCursor — instead of next() silently returning false while positionTo* threw a bare IllegalStateException.
• The DESC page of positionToLastKey is flagged and can never serve a forward move.
• The table name is resolved once per cursor instead of once per query.

Tests

• testCursorReposition — backward/in-place/between-keys repositioning, misses (including that a miss inside the page runs no query and that next() resumes on the following key), the VLV pattern (positionToKeyOrNext + positionToIndex(0)), the ChildrenCursor sibling-scan pattern, and an ascending positionToKey walk over 40 keys that must take 2 queries, not 40.
• testCursorPagingAcrossPages — page pinned to 4 rows growing to 8 over a 40 row tree: full scan across page boundaries, forward repositioning falling back to a slice when the page runs out, positionToKey/positionToIndex/positionToLastKey far beyond the first page, and next() after a positioned key across several pages.
• testCursorKeyOrderIsUnsigned — {0x7F} vs {0x80,0x01} verify the unsigned clustering order the buffer-serving logic relies on, and the reposition is now a forward move so it exercises the client-side comparison (with the second next() it used to be a backward jump served by the server).
• testCursorAfterClose — navigation on a closed cursor.
• The class needs sequential=true: TestListener rejects a class that declares test methods of its own without it.

Full local run against testcontainers: org.opends.server.backends.cassandra.TestCase 38 tests, 0 failures, 0 skipped (34 inherited PluggableBackendImplTestCase tests + the 4 cursor tests) and EncryptedTestCase 34 tests, 0 failures, 0 skipped, both with org.opends.server.TestListener enabled.

[maximthomas]

The premise checks out: PagingIterable javadoc confirms "subsequent calls to iterator() will return the same iterator instance", so the //restart iterator lines really were no-ops. This fixes three genuine wrong-results bugs (VLV-by-assertion offsets, DN2URI.targetEntryReferrals never finding referrals above the target, backward repositioning), and on every seek-shaped path the new page is a strict suffix of what master transferred. positionToLastKey going from a full partition drain to one row is a large win.

One change requested before merge; everything else is follow-up material.

positionToKey missed the buffer fast path (major)

positionToKeyOrNext gained the forward buffer walk; positionToKey did not, so every non-exact call issues a fresh page-sized slice:

// CASStorage.java:419
public boolean positionToKey(ByteSequence key) {
    if (isDefined() && key.compareTo(getKey())==0) {
        return true;
    }
    if (seek(key) && key.compareTo(getKey())==0) {   // always a new CQL query
        return true;
    }
    current=null;
    return false;
}

opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/EntryContainer.java:1724 loops it over an ascending-sorted ID list on one shared id2entry cursor (Collections.sort(entriesToBeDeleted) at :1718); renameSingleEntry at :2135 has the same shape (:2086 sorts by Pair.getPairComparator(), and positionToKey uses getFirst()):

try (final Cursor<EntryID, Entry> cursor = id2entry.openCursor(txn)) {
  for (Long entryIDLong : entriesToBeDeleted) {        // strictly ascending
    if (!cursor.positionToKey(entryID.toByteString())) { ... }

Master walked this in one forward pass. Now it is K queries, each eagerly pulling up to basic.request.page-size = 5000 rows of full entry blobs (default confirmed in java-driver-core 4.19.2 reference.conf:135; no override anywhere in the repo). Single-entry delete gets faster; a 100-entry subtree delete goes from ~5 MB to ~500 MB. That is the same amplification this PR exists to remove, one method over.

Fix is the loop already written next door:

if (isDefined()) {
    final int cmp=key.compareTo(getKey());
    if (cmp==0) return true;
    if (cmp>0) {
        while (rc.getAvailableWithoutFetching()>0) {
            current=iterator.next();
            final int c=key.compareTo(getKey());
            if (c==0) return true;
            if (c<0) { current=null; return false; }   // walked past it
        }
    }
}

Keep the key>= slice rather than switching to the point read in TransactionImpl.read — DN2ID.openCursor0 positions and then iterates with next(), and the new test asserts that contract.

Tests claim coverage they don't have (minor)

Three separate gaps in opendj-server-legacy/src/test/java/org/opends/server/backends/cassandra/TestCase.java:

testCursorKeyOrderIsUnsigned doesn't exercise the buffer path its javadoc names. The second next() leaves the cursor on high, so the reposition compares {0x80} vs {0x80,0x01} → negative (shorter prefix is less, ByteString.compareTo returns length1 - length2) → backward branch → server-side seek:

assertTrue(cursor.next());
assertEquals(cursor.getKey(), high);        // {0x80,0x01}  <-- makes the next line a backward jump
assertTrue(cursor.positionToKeyOrNext(ByteString.valueOfBytes(new byte[] { (byte) 0x80 })));

It verifies Cassandra's collation — worth keeping — but not the client-side comparison at CASStorage.java:408. Dropping the two lines above restores the intended branch.

The cross-page fallthrough is untested. With 40 rows in a 5000-row page, getAvailableWithoutFetching() is never 0 mid-scan, so CASStorage.java:406 → :415 never runs. That branch is what makes the fix correct on production-sized trees; a regression in it would pass CI.

The queryCount assertions silently depend on the default page size. assertEquals(impl.queryCount, 1) and assertTrue(impl.queryCount <= 3) only hold while one page covers all 40 rows. datastax-java-driver.basic.request.page-size is a live system property (this class already tunes the driver that way at CASStorage.java:188), so setting it to 10 fails the test with sibling scan took 5 queries — pointing at code that is behaving correctly. Pin the page size in the test.

Nits

• Failed positionToKey skips a row: seek() consumes the first row >= key, then :426 sets current=null and leaves the iterator past it, so a following next() returns the second row. Unreachable today — only DN2ID.openCursor0 ignores the return value, and ChildrenCursor/SubtreeCursor mask it via limit = ByteString.empty() — but it is a trap for the next caller. Worth a test asserting miss-then-next().
• positionToLastKey leaves a DESC ResultSet in rc: safe only because LIMIT 1 guarantees getAvailableWithoutFetching()==0. Change that LIMIT and the forward-serve loop walks rows in descending order. Worth a comment at :406, not just at :433.
• No page sizing at all: setPageSize is never called, so hasSubordinates still moves ~4000 rows to answer a 2-row question. Not a regression — master was worse — but it means this is roughly a 2x cut on the hottest path where #863 got ~100x from adaptive sizing. Suggest not describing it as closing #860 for Cassandra.
• Post-close() behaviour is inconsistent: next() returns false silently while positionTo* throw a bare IllegalStateException, which is not a StorageRuntimeException. Still an improvement over master's NPE.
• getTableName() per query: two replaceAll calls plus a concat on every select(), now once per reposition rather than once per cursor.
• Cleanup swallows everything: catch (Exception ignored) {} around the deleteTree writes leaves a stale partition with no log line.

[vharseko]

Thanks — the blocking finding was exact, including the two call sites and the arithmetic. Everything below is in f0f7f55; the PR description is updated to match.

positionToKey missed the buffer fast path (major) — fixed

positionToKeyOrNext and positionToKey now share one forwardInPage(key, exactMatch) walk, so the ascending loops on the shared id2entry cursor in EntryContainer.deleteSubtree and renameSubtree are one forward pass again. As you suggested, the key>= slice stays — DN2ID.openCursor0 positions and then iterates with next().

But that fix alone only covers the shape "one cursor, ascending keys". Two other callers pay the same page and cannot be helped by any client-side walk:

• ID2Entry.containsEntryID (ID2Entry.java:507) opens a fresh cursor per call — a page of full entries to answer a boolean.
• DN2URI.targetEntryReferrals (DN2URI.java:554) walks up the DIT, so every lookup is a backward one.

So the page itself is now sized by the cursor rather than inherited from the driver: it starts at 32 rows and doubles up to 1000 (org.openidentityplatform.opendj.cassandra.fetchsize.initial / ...fetchsize, mirroring ...jdbc.fetchsize), and a scan that runs past the end of its page continues with a keyset slice and key>:key ORDER BY key at the bigger size instead of letting the driver page at the size the cursor started with. That also answers your "no page sizing at all" nit — hasSubordinates now moves 32 rows, not ~4000.

Tests

• testCursorKeyOrderIsUnsigned — right, {0x80} vs {0x80,0x01} is negative (ByteString.compareTo falls through to length1 - length2), so it was a backward jump served by the server. The second next() is gone and the test now asserts the query count is unchanged across the reposition, which pins it to the client-side comparison.
• Cross-page fallthrough — one correction: the fallthrough itself was reached, by the positionToKeyOrNext("key99") step (the page is exhausted at the end of the partition and the code falls through to the seek). What was untested is the case that matters: the page running out mid-scan with rows still on the server. New testCursorPagingAcrossPages pins the page to 4 rows growing to 8 over a 40 row tree and covers a full next() scan across boundaries, forward repositioning falling back to a slice, and positionToKey/positionToIndex/next() far past the first page.
• queryCount depending on the page size — no longer load-bearing in that form: the cursor does not inherit datastax-java-driver.basic.request.page-size at all, and the tests pin the cursor's own bounds around the CASStorage constructor (that is where they are read). Setting the driver property can no longer break them.

Nits

• Failed positionToKey skipped a row — rather than just testing it, the behaviour is now pdb's: the miss leaves the cursor undefined but stopped just before the first key after the missing one, so next() returns that row instead of the one after it. Asserted in testCursorReposition.
• DESC ResultSet left in rc — the cursor now flags that page as descending, so it can never serve a forward move regardless of what the LIMIT becomes.
• Post-close() behaviour — uniform: a closed cursor is undefined and every navigation returns false, like EmptyCursor. No more IllegalStateException from half the methods.
• getTableName() per query — resolved once per cursor.
• Cleanup swallowing everything — it logs now, but the real protection is that each test starts by clearing its tree, so a rerun against a persistent Cassandra (the docker run flow in the file header) is not poisoned by an interrupted run.
• "closing JDBC backend: cursor repositioning re-fetches full 1000-row batches, causing excessive database network traffic #860 for Cassandra" — the description never claimed that; it says "follow-up to [#860] Serve JDBC cursor repositioning from the fetched buffer and grow batches adaptively #863/JDBC backend: cursor repositioning re-fetches full 1000-row batches, causing excessive database network traffic #860". It now spells out the paging behaviour explicitly so there is nothing to read into.

One thing the review did not cover: CI was red because of this PR

All five build-maven (ubuntu-latest, *) jobs failed on 65a0fe6:

TestCase.testCursorKeyOrderIsUnsigned » Runtime The @Test annotation for class
org.opends.server.backends.cassandra.TestCase must include sequential=true

TestListener.enforceTestClassTypeAndAnnotations (TestListener.java:632) checks the nearest class carrying @Test. While cassandra/TestCase declared no test methods of its own, that was PluggableBackendImplTestCase (which has sequential = true); adding methods to the subclass moved the check onto its own bare @Test. macOS/Windows stayed green only because Docker is absent there and the class skips. Fixed by @Test(groups = { "precommit", "pluggablebackend" }, sequential = true).

Worth remembering when running these locally: a plain TestNG run without -listener org.opends.server.TestListener cannot reproduce that class of failure.

Verification

Locally against testcontainers, with org.opends.server.TestListener enabled: org.opends.server.backends.cassandra.TestCase — 38 tests, 0 failures, 0 skipped (34 inherited + 4 cursor tests), EncryptedTestCase — 34 tests, 0 failures, 0 skipped.