-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
🐛 Destination Weaviate: Multi Tenancy Support #34229
🐛 Destination Weaviate: Multi Tenancy Support #34229
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Before Merging a Connector Pull RequestWow! What a great pull request you have here! 🎉 To merge this PR, ensure the following has been done/considered for each connector added or updated:
If the checklist is complete, but the CI check is failing,
|
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.
Hi @Marcus0086 , this looks mostly good to me, only thing missing seems to be handling the tenant id to the delete
method. Could you add that and a test for it? Then it should be ready to merge.
I bumped the version number and added the changelog entry.
Thanks for the contribution
CI PR here: #34269 |
Sure adding that |
@Marcus0086 let me know once it's ready for another look |
Hey @flash1293 , can you review the changes? |
Should I escape the deleting if a tenant is not in class tenants? This would be for multitenancy. |
@Marcus0086 What do you mean by "escape"? I would have expected that prior to deleting you add the currently configured tenant id to all the classes. Basically execute https://github.com/airbytehq/airbyte/pull/34229/files?diff=unified&w=1#diff-605751b53927fd85aee9d14cf7cc427ddc74aad7c986c31bffc1b06e528a5923R105 even if the class exists already - because we always need to make sure the tenant is added to the classes |
By ‘escape’ I wanted to mean that I would be skipping the whole deletion part if tenant is not in the class tenants list. |
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.
Adjusted the unit test but LGTM now. Merging...
Thanks for all the help |
Co-authored-by: Joe Reuter <joe@airbyte.io>
Co-authored-by: Joe Reuter <joe@airbyte.io>
What
Currently there is no way to enable multi-tenancy in weaviate through the configuration. Which disallows certain use cases like sharding and data isolation in db level.
it fixes #33992 (comment)
How
The solution is relatively simple, added an optional field in config object which can take user input as
tenant_id
If tenand_id is empty then the schema would be without multitenancy enabled and vice-versa
More on this topic: https://weaviate.io/developers/weaviate/api/rest/schema#enable-multi-tenancy
🚨 User Impact 🚨
No, there is no as such merge conflicts and breaking changes. Just a new field is added as optional field
For connector PRs, use this section to explain which type of semantic versioning bump occurs as a result of the changes. Refer to our Semantic Versioning for Connectors guidelines for more information. Breaking changes to connectors must be documented by an Airbyte engineer (PR author, or reviewer for community PRs) by using the Breaking Change Release Playbook.
If there are breaking changes, please merge this PR with the 🚨🚨 emoji so changelog authors can further highlight this if needed.
Pre-merge Actions
Expand the relevant checklist and delete the others.
New Connector
Community member or Airbyter
./gradlew :airbyte-integrations:connectors:<name>:integrationTest
.0.0.1
Dockerfile
has version0.0.1
README.md
bootstrap.md
. See description and examplesdocs/integrations/<source or destination>/<name>.md
including changelog with an entry for the initial version. See changelog exampledocs/integrations/README.md
Airbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
Updating a connector
Community member or Airbyter
Airbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
Connector Generator
-scaffold
in their name) have been updated with the latest scaffold by running./gradlew :airbyte-integrations:connector-templates:generator:generateScaffolds
then checking in your changesUpdating the Python CDK
Airbyter
Before merging:
--use-local-cdk --name=source-<connector>
as optionsairbyte-ci connectors --use-local-cdk --name=source-<connector> test
After merging: