Skip to content
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

Adds FDB Tenants #55

Closed
wants to merge 6 commits into from

Conversation

JesseStimpson
Copy link
Contributor

Hi!

This commit adds support for FDB Tenants to erlfdb, and I wanted to offer it for your consideration.

I used the source for the official FDB Python and Java clients as reference.

Further guidance needed

There are some decisions that will probably raise an eyebrow. They are:

  • Updated FDB_API_VERSION and api_version to 730, from 620.
  • Changed unit test get_empty_test to use Tenants.

API version

Tenant support exists in some version after 620, though I did not pinpoint it precisely. Presumably this would be a breaking change for anyone using a cluster with <= 620. Guidance is appreciated here.

Empty Unit Test

I almost got myself into a pickle by running unit tests using a real cluster file. Perhaps I should have known better, but I didn't expect the unit tests to run a full clear_range. (Especially given that the unit tests are set to run with a simple make command). Luckily, the cluster I used only contained dev data, so no harm done. However, this seemed like an opportunity to use tenants to conduct the test in a safe manner.

@nickva
Copy link
Contributor

nickva commented Feb 16, 2024

Thank you for your PR, @JesseStimpson!

The implementation looks reasonable. For the FDB_API_VERSION version, from what I remember, we usually target some minimal supported version explicitly. That is why we had a hard-coded version there.

Unfortunately, I don't have a current working FDB setup to validate the PR in depth. If you're willing, feel free to fork and maintain the repo. I don't think anyone has been updating or looking at this repo besides me in a while.

I almost got myself into a pickle by running unit tests using a real cluster file. Perhaps I should have known better, but I didn't expect the unit tests to run a full clear_range.

Oof. Sorry about that. Yeah, the tests usually assume they have full rein on the cluster and just wipe it out. Perhaps the new tenants feature could help with that.

@JesseStimpson
Copy link
Contributor Author

Thanks @nickva ! I've moved this work over to foundationdb-beam#1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants