Enable test concurrency by default#4313
Draft
ScottDugas wants to merge 79 commits into
Draft
Conversation
JUnit 6.1 introduced a new executor service to be used for parallel testing that does not use a ForkJoinPool. The issue with the ForkJoinPool, was that tasks waiting to join futures do not count towards the concurrency. This means that as soon as a test waited on a future, another test would start, causing more load than FDB could handle. The new service does not suffer from this issue.
This protects multiple instances in the same JVM from trying to initialize the keyspace/domain concurrently, which could conflict. This shouldn't be a real problem in production environments, but is low cost, so adding it in support of concurrent tests provides real value. There are probably additional issues that need to be resolved around multiple FRLs registering a driver in the same instance... And it may make sense to have some sort of registry so that we can better control for this, and reduce the number of tests depending on the registered driver.
This allows for running multiple tests in parallel within the same JVM, without risk of having multiple threads trying to start multiple servers against the same port. This replaces the previous per-ExternalServer approach to protecting port conflicts. This does not protect as well for concurrent ExternalServers in different processes, but currently we don't run concurrently in multiple JVMs, so this should be sufficient.
SnakeYaml is not threadsafe, so reusing the same parser when we are running tests in parallel causes bugs
This is temporary. Notably, the reason we have to lock the schema setup is because dropping a database calls deleteStore which always bumps the metadata version, even though the stores that we dropping don't cache the store header. This means that deleting a database/schema will conflict with creating a database/schema (or any other operation involving the catalog, I think).
This should be irrelevant for production, but can be quite a problem when tests are running in parallel.
Running more tests in parallel, on the same JVM we can run into cache evictions, which causes the tests to fail when the expect plans to be cached.
JUnit 6.1 introduced a new executor service to be used for parallel testing that does not use a ForkJoinPool. The issue with the ForkJoinPool, was that tasks waiting to join futures do not count towards the concurrency. This means that as soon as a test waited on a future, another test would start, causing more load than FDB could handle. The new service does not suffer from this issue.
This protects multiple instances in the same JVM from trying to initialize the keyspace/domain concurrently, which could conflict. This shouldn't be a real problem in production environments, but is low cost, so adding it in support of concurrent tests provides real value. There are probably additional issues that need to be resolved around multiple FRLs registering a driver in the same instance... And it may make sense to have some sort of registry so that we can better control for this, and reduce the number of tests depending on the registered driver.
This allows for running multiple tests in parallel within the same JVM, without risk of having multiple threads trying to start multiple servers against the same port. This replaces the previous per-ExternalServer approach to protecting port conflicts. This does not protect as well for concurrent ExternalServers in different processes, but currently we don't run concurrently in multiple JVMs, so this should be sufficient.
SnakeYaml is not threadsafe, so reusing the same parser when we are running tests in parallel causes bugs
TODO move this to testing.gradle so all modules take advantage
TODO do this for other modules that are creating using the default scheduled executor
In parallel runs, cachedVersionMaintenanceOnReadsTest fails because the previous version may be more than 2s ago
Since indexing tests do a fair amount of constant work (building indexes), and do their work at batch priority, they are more susceptible to load on the system due to concurrent transactions. Give all of them a ResourceLock so that we don't have multiple tests of this style running concurrently.
TODO check if we can change this to the same resource lock as the indexing tests instead
Since createPath now evaluates the path, it can bump the cached read version, which breaks the expectation of the test TODO can we decrease the 60s back to 2s like it was before
This still runs into DeadlineExceededException, more investigation is probably needed, but this gets the tests to pass better.
JUnit 6.1 introduced a new executor service to be used for parallel testing that does not use a ForkJoinPool. The issue with the ForkJoinPool, was that tasks waiting to join futures do not count towards the concurrency. This means that as soon as a test waited on a future, another test would start, causing more load than FDB could handle. The new service does not suffer from this issue.
This protects multiple instances in the same JVM from trying to initialize the keyspace/domain concurrently, which could conflict. This shouldn't be a real problem in production environments, but is low cost, so adding it in support of concurrent tests provides real value. There are probably additional issues that need to be resolved around multiple FRLs registering a driver in the same instance... And it may make sense to have some sort of registry so that we can better control for this, and reduce the number of tests depending on the registered driver.
This allows for running multiple tests in parallel within the same JVM, without risk of having multiple threads trying to start multiple servers against the same port. This replaces the previous per-ExternalServer approach to protecting port conflicts. This does not protect as well for concurrent ExternalServers in different processes, but currently we don't run concurrently in multiple JVMs, so this should be sufficient.
SnakeYaml is not threadsafe, so reusing the same parser when we are running tests in parallel causes bugs
These overwhelm FDB, so mark them as SAME-THREAD
Among many other things this brings the fix 2 parallelism
Each test class in fdb-relational-jdbc was writing to a hardcoded database
path (e.g. /FRL/jdbc_test_db, /FRL/SimpleDirectAccessInsertionTests) on the
shared FDB cluster with no serialization around the catalog DDL, so parallel
Gradle workers — and even parallel classes in one JVM — would collide on the
same rows in /__SYS/CATALOG and either see stale state or hit SQLSTATE 40001
serialization conflicts.
Refactor JDBCAutoCommitTest, JDBCSimpleStatementTest,
JDBCParameterizedQueryComparisonTest, and SimpleDirectAccessInsertionTests
to the same shape used elsewhere in the tree:
* Per-instance database and schema-template names generated from a random
64-bit suffix in the constructor, so no two test instances ever pick the
same path.
* Setup and teardown wrapped in CatalogOperations.runLockedWithRetry so
every CREATE/DROP participates in the JVM-wide catalog monitor and gets
retried on SQLSTATE 40001.
* SELECT-from-databases assertions filtered to just the test's own DB +
/__SYS, so a sibling class's in-flight database doesn't skew the row
count or ordering.
* Cleanup uses IF EXISTS variants (and DROP SCHEMA TEMPLATE only, since
DROP DATABASE cascades and the DDL parser silently ignores
DROP SCHEMA IF EXISTS).
RelationalServerTest.simpleJDBCServiceClientOperation(ManagedChannel) was
using a hardcoded /FRL/server_test_db path and issuing catalog DDL directly
over gRPC with no serialization. Running it alongside the fdb-relational-jdbc
tests on the same FDB cluster hit SQLSTATE 40001 conflicts on /__SYS/CATALOG.
Refactor the helper to match the pattern now used by the jdbc tests:
* Per-invocation random suffix for the database and schema-template names.
* Setup and cleanup wrapped in CatalogOperations.runLockedWithRetry, with
IF EXISTS defensively dropping any prior state on the way in.
* The "select * from databases" assertion filters to just this test's DB +
/__SYS so a sibling class's leftover database can't skew the row count.
* Cleanup moved into a finally so a mid-test failure still leaves the
catalog clean.
The helper now throws SQLException, so its callers — RelationalServerTest's
simpleJDBCServiceClientOperation()/testMetrics() and InProcessRelationalServerTest's
override — declare it too. The testMetrics grpc-call-count assertion is
bumped from 7.0 to 8.0 to account for the extra DROP SCHEMA TEMPLATE IF EXISTS
now issued during setup (5 setup + 1 execute + 2 cleanup).
RecordLayerStoreCatalogImplTest and RecordLayerStoreCatalogWithNoTemplateOperationsTest both call FDBRecordStore.deleteStore on /__SYS/CATALOG in @AfterEach — literally the record store every relational test depends on. They were previously marked @isolated, which only serialises inside the JVM; a sibling Gradle worker JVM could still see the catalog vanish from under its own tests, or write its own schema to the /__SYS record store just as these tests were about to assert "exactly one schema exists". The correct tag for tests that wipe global FDB state is @tag(Tags.WipesFDB). The destructiveTest Gradle task includes WipesFDB with maxParallelForks = 1, giving cross-JVM serialisation on top of same-JVM isolation. The tag also routes these tests out of the normal test task so they don't slow every CI build. Hoist the shared setup/teardown lifecycle into the abstract base: * @tag(Tags.WipesFDB) is declared once on RecordLayerStoreCatalogTestBase. JUnit propagates class-level tags down the inheritance chain, so both concrete subclasses pick it up without redeclaring — see ExtendedDirectoryLayerTest / LocatableResolverTest for the same pattern already in use in fdb-record-layer-core. * @beforeeach in the base opens the FDB, wipes any leftover catalog, then calls a new abstract createCatalog(txn) that subclasses implement to pick between StoreCatalogProvider.getCatalog(...) and getCatalogWithNoTemplateOperations(...). * @AfterEach delegates to the same private deleteCatalog() helper. The extra pre-test wipe is defensive against a previous run that crashed or was killed between phases and left orphan catalog state behind. * Both concrete subclasses drop their @isolated, their imports for FDBDatabaseFactory / FDBRecordStore / KeySpacePath / RelationalKeyspaceProvider / FDBTestEnvironment / Tags, and their duplicated @beforeeach / @AfterEach / deleteCatalog blocks — they now contribute only createCatalog() plus their variant-specific @tests.
Parallel test execution surfaced a latent race in AgilityContextTest.testReadOnlyHasCallerGrv. The test depends on earlyContext's read version being pinned *before* laterContext committed. Nothing in the test pinned that GRV explicitly. Instead the test relied on the side effect of `path.toSubspace(earlyContext)` — which resolves DirectoryLayer entries and, if any of them need an FDB round-trip, forces earlyContext to grab a GRV in the process. But those resolutions are JVM-cached, so once any sibling test in the same worker JVM has warmed the resolver for this test's path prefix, toSubspace() returns from memory without touching FDB and earlyContext's GRV stays unset. The GRV is then first grabbed inside ReadOnlyNonAgileContext.ctor via callerContext.getReadVersion() — which now happens after laterContext has already committed, so earlyContext (and the derived AgilityContext) both see the write and the assertion fails. Fix: call earlyContext.getReadVersion() immediately after openContext(), before laterContext is opened. That pins the read version deterministically regardless of what the directory-layer cache does.
Three tests were failing under parallel test execution with allocator
IllegalStateException:
* FDBRecordStoreReplaceIndexTest.buildReplacementsInMultipleStores
* VersionIndexTest.deleteStoreWithUncommittedVersionData
[FORMAT_CONTROL, true, true]
* FDBReverseDirectoryCacheTest.testReverseDirectoryCacheLookup
All three trip the root HighContentionAllocator's hasConflictAtRoot check
when a randomly-chosen candidate byte prefix already has keys under it —
which happens when sibling tests are concurrently writing at low-integer
prefixes.
Fix without moving the tests to destructiveTest:
* TestKeySpace.STORE_PATH: change from DirectoryLayerDirectory to a
KeyType.LONG KeySpaceDirectory. STORE_PATH had only two callers
(FDBRecordStoreReplaceIndexTest, VersionIndexTest); both were passing
fixed strings ("store_0", "path1", …) that got interned via the shared
root DL. Passing raw longs skips the allocator entirely — the id
encodes directly into the tuple.
* FDBReverseDirectoryCacheTest.testReverseDirectoryCacheLookup: this
one legitimately exercises the shared globalScope, so add a
resolveWithRetry helper that catches the specific IllegalStateException
the FRL HCA and re-invokes globalScope.resolve — the HCA picks a fresh
random candidate each attempt, so the retry converges quickly.
createRandomDirectoryEntries uses the helper; other test-body resolves
look up already-created names and hit the cached forward mapping without
allocating, so they don't need the retry.
…hContinuationTest This test is getting transaction-too-old now that we have parallelism, try just decreasing the batch size a bit.
RecordTypeKeyTest.testScanningWithUnknownKeys — and, sporadically, any
other test that opens an FDB transaction — failed under parallel test
execution with:
IllegalStateException: Cannot access closed object
at NativeObjectWrapper.getPtr
at FDBDatabase.createTransaction
EmbeddedRelationalExtension.makeDatabase was:
try (var connection = new DirectFdbConnection(database);
Transaction txn = ...) { ... }
DirectFdbConnection.close() calls fdb.close() on the underlying FDBDatabase,
but the `database` field here holds the shared handle returned by
FDBDatabaseFactory.instance().getDatabase(clusterFile) — the same JVM-wide
handle every other test is using. Every extension @beforeeach closed that
shared handle. Any sibling test that reached NativeObjectWrapper.getPtr
during that window hit "Cannot access closed object", which the test's
assertion helper reported as a mismatch with the expected SQLException.
Fix: pull `connection` out of the try-with-resources — only close the
Transaction. The DirectFdbConnection wrapper is lightweight; the
FDBDatabase belongs to the factory cache and must stay open for the
JVM's lifetime.
This sets Everything annotated with @yamltest to run the test methods concurrently, but adds a resource-lock so that the individual parameterizations of each method run serially
…nStamp In particular the catalog uses the MetaDataVersion-based store cache, and when you delete a schema (as part of a test cleanup), the old deleteStore would invalidate the metadataversion cache, which would cause any catalog operations (which opened the store) to conflict. By being more restrictive, deleting stores that aren't caching the header means that we won't force a refresh on stores that are using the store header cache.
This should prevent conflicts with the external servers, and initial catalog operations.
This mutates the singleton keyspace, so should be synchronized
Changes to the production code mean that the test code no longer needs to synchronize/retry.
Temporary additional logging on the plan cache to expose more information because sometimes tests fail because they have a cache miss. I haven't reproduced with the logging but saving that for now.
Add three related tests to FDBRecordStoreStateCacheTest that pin the
interaction between the state cache and FDB's read-conflict tracking
in FDBRecordStore.loadStoreHeaderAsync.
cacheMissOpenConflictsWithConcurrentHeaderWrite
Miss path takes loadStoreHeaderAsync, which issues a SERIALIZABLE
getRange(subspace.range(), 1). The resulting read conflict covers
the store header key. A concurrent commit that writes STORE_INFO_KEY
(setHeaderUserField here) fails the reader with
FDBStoreTransactionConflictException.
cacheHitOpenDoesNotConflictWithConcurrentRecordInsert
When the cache serves the open,
FDBRecordStoreStateCacheEntry.handleCachedState only adds a point
read conflict on STORE_INFO_KEY. A concurrent record insert that
writes into <subspace>/RECORDS/... does not overlap, so the reader
commits cleanly.
concurrentCacheMissOpensBothInsertingRecordsDoNotConflict
Two concurrent cache-miss opens that both only insert records (no
header mutation) commit cleanly. This is the surprising one: it
rules out a natural-sounding hypothesis — that the SERIALIZABLE
getRange in loadStoreHeaderAsync adds subspace.range() as a
read-conflict range and thereby conflicts with any concurrent write
inside that subspace. If that were true, the two record inserts
here would collide; they don't, so the miss-path read conflict
stops at (or near) the store header key.
Motivation: these tests were written while trying to explain
/__SYS/CATALOG SERIALIZATION_FAILUREs observed in the parallel
yaml-tests harness. The third test disproves the leading hypothesis
that the reported subspace-wide conflict range came from the miss-path
read alone. The mechanism producing that conflict shape is still not
identified, but any future explanation now has to be consistent with
these three assertions.
…rstKey
FDBRecordStore.readStoreFirstKey previously issued
context.readTransaction(isolationLevel.isSnapshot())
.getRange(subspace.range(), 1)
which, under SERIALIZABLE isolation, relied on FDB's Java bindings to
add a read-conflict range implicitly. Empirically, under heavy
concurrent load that implicit range can widen to cover the entire
store subspace, causing spurious SERIALIZATION_FAILUREs against any
concurrent writer inside that subspace — the shape observed for the
/__SYS/CATALOG conflicts in the parallel yaml-tests harness. The
exact mechanism by which the implicit range widens is not fully
understood.
Now always read at SNAPSHOT so no implicit read-conflict range is
added, then, if the caller passed a non-snapshot isolation level,
explicitly add a deterministic read-conflict range:
- [subspace.range().begin, firstKey + 0x00) if a key was found,
- the full subspace.range() if the store is empty
(so the load still serialises against a concurrent creator).
Effect on yaml-tests: catalog SERIALIZATION_FAILUREs are meaningfully
less frequent; a lingering hit occasionally still fires from another
(currently unidentified) code path but is absorbed by retry.
That being said, this doesn't align with analysis of the actual FDB
code, and it should be doing what we would expect without this change.
Another possibility, since this is yaml-tests, is that the external
server (old version), didn't have the change to deleteStore, and
we are doing deleteStore with that, and it is manifesting as a
conflict range here, even though it shouldn't. Or there's something
else wrong, and more testing will show that this did not fix the issue.
the goal here is to detect whatever is touching the catalog's entire range causing read conflicts in other tests. There wasn't really any success, but maybe with more runs it will show something.
I've already seen a parallelism of 4 fail in CI, but on a more powerful laptop, it is needed to hit concurrency issues.
Conflicts in fdb-record-layer-core/fdb-record-layer-core.gradle In main, I added testFixtures, and here we add the mockito javaagent
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
1 participant
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.
No description provided.