Enable test concurrency by default#4313
Draft
ScottDugas wants to merge 63 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
Registering and de-regestering FRL causes issues with running the tests in parallel. Instead have the relational extension provide a driver for tests and other extensions to use.
The catalog tests have to wipe and recreate the catalog, so those must be isolated from all other relational tests.
Tests that assert about logs must be run by themselves, because the log is shared.
This ends up being a lot, it may make more sense to fix the production behavior so that deleting a schema doesn't cause problems. Or, change our tests so that they don't clean up anything until after all tests are done (or ever).
The switch to not registering the driver caused it to directly call connect() on the driver, but the contract for that is different, namely, the driver is supposed to return null for something it doesn't understand, and the DriverManager converts that to an exception. Update the test to expect the actual contract.
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 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.