chore: add cockroachdb 26.1 to the matrix and modify version parsing#2907
chore: add cockroachdb 26.1 to the matrix and modify version parsing#2907tstirrat15 merged 1 commit intomainfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project check has failed because the head coverage (74.54%) is below the target coverage (75.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #2907 +/- ##
==========================================
+ Coverage 73.57% 74.54% +0.98%
==========================================
Files 489 489
Lines 60373 60365 -8
==========================================
+ Hits 44414 44995 +581
+ Misses 12798 12218 -580
+ Partials 3161 3152 -9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
128c01f to
05fdf9e
Compare
0af5c22 to
22651c5
Compare
ae66de5 to
3406d66
Compare
3406d66 to
6500d9b
Compare
internal/datastore/crdb/crdb_test.go
Outdated
| WithAcquireTimeout(5*time.Second), | ||
| )) | ||
|
|
||
| t.Run("TestVersionReading", createDatastoreTest( |
There was a problem hiding this comment.
why wrap VersionReadingTest at all with createDatastoreTest? just write it as a normal test func TestVersionReading(t *testing.T) { .. }
There was a problem hiding this comment.
Because the createDatastoreTest is parametrized over CRDB versions and takes care of setting up the datastore. I want to be sure that we're correctly reading the version for all CRDB versions that we're testing against.
| const ( | ||
| readMaxConns = 10 | ||
| ) | ||
| // Set up a raw connection to the DB |
There was a problem hiding this comment.
to what DB? where was the DB created?
There was a problem hiding this comment.
The DB instance in the test? Or are you wanting a more descriptive comment?
There was a problem hiding this comment.
the DB is created in createDatastoreTest, and the url that we hardcoded in line 986 just... works? is that it?
i wonder if we could structure this better. e.g. if the DB created points to port 5433, this test will break.
There was a problem hiding this comment.
I think so - I copied this test from the structure of a preceding test. I haven't dug into how they work internally. I kinda wonder if this is something with a pgx test connection or a detail of how we're using test containers.
4e0383b to
16f487e
Compare
|
Maybe refactor to use |
|
^ That would require admin, which would probably break OSS users of CRDB, so we won't go that route. |
e3c3f0d to
7f82bf5
Compare
There was a problem hiding this comment.
These are the salient changes.
internal/datastore/crdb/crdb_test.go
Outdated
| require.Equal(t, float64(readMaxConns), poolReadMetric.GetGauge().GetValue()) //nolint:testifylint // we expect exact values | ||
| } | ||
|
|
||
| func VersionReadingTest(t *testing.T, _ datastore.Datastore) { |
There was a problem hiding this comment.
This is a net-new integration test that asserts that the version requested by the env var is correctly parsed by the internal logic.
|
|
||
| // RevisionQuantizationTest tests whether or not the requirements for revisions hold | ||
| // for a particular datastore. | ||
| // TODO: rewrite using synctest |
There was a problem hiding this comment.
I removed this TODO because I figured out that this is interacting with an external database whose time state can't be captured by a synctest bubble.
| // assert that recent call to head revision is also valid, even after a GC window cycle without writes elapsed | ||
| require.NoError(ds.CheckRevision(ctx, head), "expected freshly obtained head revision to be valid") |
There was a problem hiding this comment.
Moved this up, since we don't want the length of time that the namespace reads to cause this assertion to flap (which is what was happening to me locally)
| // TODO: these reads are taking a significant amount of time on CRDB, on the order | ||
| // of 100ms for a row read. We need to ascertain whether this is a test artifact | ||
| // or a performance regression. |
There was a problem hiding this comment.
This is something that we should figure out but isn't related to the changes in this PR.
99244b9 to
04d02af
Compare
pkg/datastore/test/watch.go
Outdated
| // This should mean that we can wait until the checkpoint is emitted and then | ||
| // make our writes. | ||
| // CheckpointInterval: 1*time.Second, |
There was a problem hiding this comment.
| // This should mean that we can wait until the checkpoint is emitted and then | |
| // make our writes. | |
| // CheckpointInterval: 1*time.Second, |
04d02af to
82b9711
Compare
82b9711 to
1f55937
Compare
Description
Cockroach 26.1 has been released and we want to make sure that SpiceDB plays nice with it. This moves the matrix so that we test against 26.1.
We found that Cockroach now restricts access to its internal table, so we can't grab the version object directly. This changes the logic so that we rely on string parsing. This is less safe/direct, but it also works with v26.1.
We also found that checkpointing behavior changed in 26.1, so we updated
min_checkpoint_interval.We also
Changes
min_checkpoint_intervalin 26.1Testing
Review. See that tests pass.