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

feat (sync-service): multi tenancy support #1868

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from
Draft

Conversation

alco
Copy link
Member

@alco alco commented Oct 17, 2024

Supersedes #1844. I kept that PR up to consult its diff until all the issues have been resolved here.

This PR implements #1591.
It enables Electric to work with multiple databases (i.e. tenants).

TODO:

  • rebase on top of main
  • review commented out code and either remove it or make sure the place it had been moved to has been updated for multitenancy
  • Implement a two-tier ETS storage for column info and relation data. In this PR we replace two named tables with anonymous ones, necessitating a roundtrip to a single gen server just to fetch the table handle. This gen server becomes a bottleneck. We should either create another static ETS table to lookup individual ETS table handles in or store all column info and relation info in two static (named) tables, ensuring they don't trump each other via proper key design.
  • make unit tests for multi tenancy
  • add a diagram explaining the new architecture
  • update the open API spec

@alco
Copy link
Member Author

alco commented Oct 17, 2024

@kevin-dp This is where this PR is currently at after I have rebased it on top of main:

  • Elixir unit tests all pass
  • lux-based integration tests fail. We need to create a test tenant for integration tests
  • TS-based integration tests fail for the same reason: missing test tenant
  • there's a block of commented-out test code I haven't yet resolved. Those are tests that were moved to other modules after you had started the original multi-tenancy PR. We just need to make sure that those moved tests haven't been lost in the rebasing process.

@@ -192,6 +196,51 @@ defmodule Electric.Plug.ServeShapePlug do
# end_telemetry_span needs to always be the last plug here.
plug :end_telemetry_span

defp validate_tenant_id(%Conn{} = conn, _) do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tenant_id needs to be a UUID so we need to validate that here.

On CI we don't wait for the database to start accepting connections
before running Electric. And we cannot check the response body of the
health check request. So adjusting the response status is the only
remaining option.
@alco
Copy link
Member Author

alco commented Oct 17, 2024

I've replaced the test tenant with a configurable default tenant that works in any environment. All tests are green now.

The code still needs a good cleanup and deduplication pass. I think we can avoid trickling electric_instance_id and tenant_id down to so many module by passing a partially-applied process_name() function under the name option instead.

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.

3 participants