Repository navigation
fix(storage): make the cassandradb provider work on apache cassandra - #821
Closed
lakhansamani wants to merge 2 commits into
Closed
lakhansamani wants to merge 2 commits into
lakhansamani wants to merge 2 commits into
Conversation
The provider backs two DB types, cassandradb and scylladb, but only the
ScyllaDB path was ever tested -- cassandradb was absent from the storage
suite's backend list. Wiring it in and pointing it at cassandra:4.1 fails
immediately: three queries use CQL LIKE, which Apache Cassandra accepts
only against a SASI index and rejects against the plain secondary index
this provider creates.
LIKE restriction is only supported on properly indexed columns
ScyllaDB accepts the same statements, which is why it went unnoticed.
- GetWebhookByEventName errored out on every Apache Cassandra
deployment, so webhook dispatch by event name never fired.
- DeleteAllSessionTokensByUserID and DeleteSessionTokensByNamespace
errored the same way.
All three already carried ALLOW FILTERING -- a full table scan -- so
moving the comparison into Go changes nothing but where it runs.
DeleteAllSessionTokensByUserID also matched "%<userId>%" unanchored, so
a delete for "42" took out tokens for "142" or "provider:427". The SQL
provider was anchored long ago; this one kept the defect because the
assertion covering it lived inside a SQL-only subtest. It now runs on
every backend as Anchored Session Token Delete, and fails on ScyllaDB
when the unanchored match is restored.
Also stops Webhook_Operations indexing a possibly-empty result: the
panic aborted the whole run for every backend queued behind it.
Adds make test-cassandradb and includes cassandradb in test-all-db.
Full storage suite now green on cassandra:4.1 and on scylladb.
Two self-inflicted failures that look like storage bugs. Both are invisible in CI, which gets a fresh container per run, and deterministic on any repeated local run -- the "flaky ScyllaDB storage tests" shape. User_Operations asserted the just-created user appears in ListUsers(Limit:10, Offset:0) *unfiltered*. That only holds while the table is nearly empty. At 463 accumulated rows it failed 15 of 15 runs with "created user should appear in ListUsers", i.e. exactly like a write that never landed. Now filtered by the user's own email, which keeps both assertions and makes them independent of how much data is already there. The two phone numbers were fixed literals, so the failure above -- which aborts before the cleanup at the bottom -- left +1234567891 in the table and every later run failed earlier with "user with given phone number already exists". One failure cascading into a different, misleading one. Unique per run now, like the emails. Verified against the same 463-row keyspace: was 15/15 failing, now 5/5 passing. Refs #813
Contributor
Author
|
Superseded by #824, which carries this change unmodified alongside the other three commits in one PR — no merge-order dependency between them. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Answering "did we run a local Cassandra test?" — we never had.
cassandradbwas missing from the storage suite's backend list, so the provider that serves bothcassandradbandscylladbwas only ever exercised through Scylla. Wiring it up and pointing it atcassandra:4.1fails on the first run.What was broken
Three queries use CQL
LIKE. Apache Cassandra acceptsLIKEonly against a SASI index and rejects it against the plain secondary index this provider creates:ScyllaDB accepts the same statements, which is exactly why this survived.
GetWebhookByEventNameDeleteAllSessionTokensByUserIDDeleteSessionTokensByNamespaceAll three already carried
ALLOW FILTERING, i.e. a full table scan, so the index was never doing any work. Moving the comparison into Go changes nothing but where it runs — no new scans, no behaviour change on Scylla.A second bug, on both engines
DeleteAllSessionTokensByUserIDmatched"%" + userId + "%"— unanchored. A delete for42also took out tokens stored for142orprovider:427. The SQL provider was anchored long ago (user_id = ? OR user_id LIKE '%:' || ?) with a comment explaining precisely this; the Cassandra provider kept the defect, because the assertion covering it sat insidetestSQLCRUDCorrectnessFixes, which only runs for SQL backends.It is now
Anchored Session Token Delete, running on every backend. Mutation-verified: restoring the unanchored match fails it on ScyllaDB withsubstring-matching other user's session token must survive.Also
Webhook_OperationsindexedfetchedByEventName[0]after only anassert.NotNil. An empty-but-non-nil slice panics there, and a panic in one backend's subtest aborts the run for every backend queued behind it — which is how the first Cassandra run ended, hiding whatever came after. Nowrequire.NotEmpty.Coverage
cassandradbadded toallDBTypesandgetTestDBConfig, on port 9142 — deliberately not 9042, soTEST_DBS=scylladb,cassandradbcan't bind both types to one engine and silently test Scylla twice.make test-cassandradb, same capture-status-then-teardown shape as the other single-DB targets.cassandradbadded totest-all-db,test-docker-up,test-cleanup,wait-for-test-dbs.sh, and theAGENTS.mdtable (7 DBs → 8).Verification
make test-cassandradbequivalent againstcassandra:4.1: all 26 subtests pass (was: panic at subtest 9).TEST_DBS=scylladbfull storage suite: pass.TEST_DBS=sqlite go test -p 1 ./...: pass.go build ./...,go vet ./...,gofmtclean.Second commit: the storage suite assumed an almost-empty table
Chasing #813's remaining failure, I ran the scylladb storage suite in a loop and hit 15 consecutive full-suite failures — on
mainand on pre-#818. Neither was the LWT bug; both were the test's own fault, and both are the "flaky ScyllaDB storage tests" shape #813 reports.1.
ListUsersfirst-page assumption.User_Operationsasserted the just-created user appears inListUsers(Limit: 10, Offset: 0)unfiltered. That only holds while the table is nearly empty. At 463 accumulated rows it failed every run withcreated user should appear in ListUsers— indistinguishable from a write that never landed. Now filtered by the user's own email, which keeps both assertions (ListUsers works; the created user carries its password) and makes them independent of how much data is already there.2. Cascading pollution from fixed phone literals. The failure above aborts before the cleanup at the bottom of the subtest, leaving
+1234567891in the table. Every later run then failed earlier, withuser with given phone number already exists— one failure turning into a different, misleading one. Both numbers are unique per run now, like the emails already were.Invisible in CI, which gets a fresh container per run. Deterministic locally.
Verification
Same ScyllaDB container, keyspace holding 463+ users throughout:
User_SCIM_Fields/Org_Domain_Operations/Session_Token_Operations× 100Plus the Apache Cassandra suite (26/26) and
TEST_DBS=sqlite go test -p 1 ./..., both green.