-
Notifications
You must be signed in to change notification settings - Fork 112
Add support for testing against multiple FDB clusters #3575
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
This changes the way cluster files are specified for the tests, introducing a new yaml file. This makes it more amenable to having more than one cluster file. The fdb-environment.properties is still supported so developers don't immediately have to update their configuration.
This took a lot of trial and error in CI, and adding and removing a lot of logging and investigative code. The key thing that tripped me up was the fact that you have to do `configure new ssd` if it is a new cluster, which we were not doing previously, because the installation created the cluster.
This might be a bit of overkill, if they aren't using FDBDatabaseExtension, but it probably avoids confusion later on.
it was trying to connect to the default, which doesn't exist
…stCopyInSameDatabase
The reason for the issue is that it was reassigning `fdb = dbExtension.getDatabase()`, but it loaded data into `fdb` in a `beforeEach`, so if it picked different clusters it wouldn't have any data, and execute wouldn't fail. In debugging I also removed teh second `openRecordStore`, because the state should respond to calling `markIndexWriteOnly`, you shouldn't have to re-read, and we actually say you should never open the same store with different objects in the same transaction. I also isolated the exception check, and switched to use assertJ for the assertion, because it ends up being quite a bit cleaner than with junit's assertions.
I tried putting it in testFixtures, but that doesn't play well with other sub-projects already depending on the tests configuration
Doing so requires moving everything to testFixtures, so best to do in a separate PR
… it uses in setup
I think in the long term we don't want the driver to work like this, but I'm not sure how we want to have it work, and creating an api for that seems like more work than it is worth at this time.
…tions The external yaml connections do not support some operations that aren't really needed in mixed-mode, so that needs the cluster file from the connection.
8611ab5
to
dae9a82
Compare
I think people could be confused by getting different clusters each call to `getDatabase`, so this persists that. If you want to test against multiple clusters, use the indexed methods.
factory.clear(); | ||
|
||
FDBDatabase database = factory.getDatabase(); | ||
FDBDatabase database = factory.getDatabase(FDBTestEnvironment.randomClusterFile()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test could probably just use dbExtension.getDatabase()
, but I didn't want to change too much in this PR. And you have to take care to make sure you're not reusing a database created in the setup.
throw e.getCause(); | ||
} | ||
}); | ||
void writeOnlyRankQuery() throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test was failing, and reworking it to isolate the assertThrows helped me debug what was wrong, and I felt like it would be good to keep it.
testImplementation(libs.bundles.test.runtime) | ||
testCompileOnly(libs.bundles.test.compileOnly) | ||
|
||
testImplementation project(path: ':fdb-extensions', configuration: 'tests') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about putting the new class in a testFixtures
, but I couldn't get that to work without putting all the classes that anyone already depends on via this approach in the testFixture, and thought that would be better as a followup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I think it would be better to move everything to a testFixtures
. I'll do that in a separate PR
// TODO: Make this multi-query/-tenant/-database! | ||
try { | ||
frl = new FRL(); | ||
frl = new FRL(com.apple.foundationdb.relational.api.Options.NONE, clusterFile); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also have Options
from apache cli, so this needs to be fully qualified (or statically imported)
<T> T executeTransactionally(SQLFunction<YamlConnection, T> transactionalWork) throws SQLException, RelationalException; | ||
|
||
/** | ||
* The Cluster File that this connection is connected to, so that other connections can point to the same |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not super committed to this API, but at some point we'll want multi-cluster testing in yaml-tests, and I think that's a better time to really think about it.
import java.util.Map; | ||
import java.util.concurrent.ThreadLocalRandom; | ||
|
||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about putting this in testFixtures
, but since other projects already depend on the tests configuration, doing so would require moving all the other classes into a testFixtures
, and I figured that would be better as a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made a change in main to move them into a test fixture, and then went to test publishing of testFixtures
so that yaml-tests
could be used by clients for their tests, but it looks like we can't publish testFixtures
: e9db5c0#diff-9515853bb3a0c957c759e9c453a9e17941f4333751715e75d74efed681fadac0R60 I'm going to look into creating a test-utils
library....
fdb-extensions now has a testFixtures. Put the new FDBTestEnvironment there
- the fixtures need snakeyaml - I had a typo in testFixtures
This changes the way we specify cluster files in tests. It replaces the
fdb-environment.properties
withfdb-environment.yaml
. The new format allows for specifying multiple cluster files.The majority of tests now randomly chose a file at the start of the test from the list of the available ones.
The following sub-projects set the environment variable in the gradle task because getting the project under test to support custom cluster files seemed like a bigger task:
Our CI builds also, now, setup two FDB clusters, and create an
fdb-environment.properties
.I added one new "test" to validate this all works. It connects to two different clusters. It
assume
s that there are at least two clusters. At some point in the future we will want to make that a stronger contract to ensure that a bug in our CI setup doesn't cause us to not run all the multi-cluster tests.I created Draft PR: #3623 to ensure that everything still works if you don't specify
fdb-environment.yaml
I also created this PR off the same version of main to confirm the test counts didn't change: #3621 Apparently we do merge main to do the tests so PR #3621 saw fewer tests than this branch when it was last run. I re-ran and they may be the same depending on whether new tests were merged. I validated some of the differences, and they look to be expected.
Resolves: #3574