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

cosmos spring multi-tenant samples #663

Conversation

TheovanKraay
Copy link
Contributor

Purpose

Adding samples for multi-tenancy in Spring Cosmos DB for database and container level isolation.

These are based on PRs for:

container level multi-tenancy: Azure/azure-sdk-for-java#33400
database level multi-tenancy: Azure/azure-sdk-for-java#32516

  • ...

Does this introduce a breaking change?

[ ] Yes
[x] No

Pull Request Type

What kind of change does this Pull Request introduce?

[ ] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Documentation content changes
[x] Other... Please describe: code samples

How to Test

  • Get the code
git clone [repo-address]
cd [repo-name]
git checkout [branch-name]
npm install
  • Test the code

What to Check

Verify that the following are valid

  • ...

Other Information

@TheovanKraay TheovanKraay requested a review from a team as a code owner February 24, 2023 18:44
@trande4884
Copy link
Contributor

Should we move common files like those for User, Order and TenantStorage to a common directory that can be shared to reduce duplication? Or do we want each to be fully self-contained so we do not do that?

@TheovanKraay
Copy link
Contributor Author

Should we move common files like those for User, Order and TenantStorage to a common directory that can be shared to reduce duplication? Or do we want each to be fully self-contained so we do not do that?

Just because this is a sample, prefer it to be a bit more contrived than usual.

Copy link

@chenrujun chenrujun left a comment

Choose a reason for hiding this comment

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

I left some comments about the first sample. The comments can apply to both of the 2 samples. Please update the 2 samples according to my comments.

containers = database.readAllContainers();
String msg="Listing containers in tenants database:\n";
containers.byPage(100).flatMap(readAllContainersResponse -> {
logger.info("read {} containers(s) with request charge of {}", readAllContainersResponse.getResults().size(),readAllContainersResponse.getRequestCharge());

Choose a reason for hiding this comment

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

read -> Read.

Copy link

@chenrujun chenrujun left a comment

Choose a reason for hiding this comment

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

I left some comments about the first sample. The comments can apply to both of the 2 samples. Please update the 2 samples according to my comments.

@chenrujun
Copy link

Hi, @TheovanKraay

container level multi-tenancy: Azure/azure-sdk-for-java#33400
database level multi-tenancy: Azure/azure-sdk-for-java#32516

Is it possible to demonstrate the 2 features by spring profile instead of 2 sample projects?
Here is an example: https://github.com/chenrujun/service-bus-dead-letter-queue-sample/blob/e2ab89fde4b6fce2dbb4163efc50a2a4c7d3a2a3/README.md#receive-message-by-jmslistener

@TheovanKraay
Copy link
Contributor Author

Hi, @TheovanKraay

container level multi-tenancy: Azure/azure-sdk-for-java#33400
database level multi-tenancy: Azure/azure-sdk-for-java#32516

Is it possible to demonstrate the 2 features by spring profile instead of 2 sample projects? Here is an example: https://github.com/chenrujun/service-bus-dead-letter-queue-sample/blob/e2ab89fde4b6fce2dbb4163efc50a2a4c7d3a2a3/README.md#receive-message-by-jmslistener

Whilst I like the idea, can we keep it as it is for now? It will be a lot of effort to re-factor. Also, in practice, changing tenant isolation level also changes how you will architect the data model (e.g. co-locating types and different throughput settings etc), and I think one is likely to go one way or another for a given multi-tenant app, but not both. So, I am not sure real-world apps will have different profiles for different isolation levels using Cosmos. @trande4884 any thoughts on this?

@TheovanKraay
Copy link
Contributor Author

I left some comments about the first sample. The comments can apply to both of the 2 samples. Please update the 2 samples according to my comments.

Ok, I will address them.

@chenrujun
Copy link

@TheovanKraay

Whilst I like the idea, can we keep it as it is for now? It will be a lot of effort to re-factor.

OK.

@trande4884
Copy link
Contributor

Hi, @TheovanKraay

container level multi-tenancy: Azure/azure-sdk-for-java#33400
database level multi-tenancy: Azure/azure-sdk-for-java#32516

Is it possible to demonstrate the 2 features by spring profile instead of 2 sample projects? Here is an example: https://github.com/chenrujun/service-bus-dead-letter-queue-sample/blob/e2ab89fde4b6fce2dbb4163efc50a2a4c7d3a2a3/README.md#receive-message-by-jmslistener

Whilst I like the idea, can we keep it as it is for now? It will be a lot of effort to re-factor. Also, in practice, changing tenant isolation level also changes how you will architect the data model (e.g. co-locating types and different throughput settings etc), and I think one is likely to go one way or another for a given multi-tenant app, but not both. So, I am not sure real-world apps will have different profiles for different isolation levels using Cosmos. @trande4884 any thoughts on this?

I agree to hold off on the Profile change. I think it could be discussed for a future PR if we want to go that route, but I have not seen any activity from our users on Profile usage so I'm not sure that is the best route to go in our samples. I also just like the standalone samples as they are very clear to the user as to what is needed.

1. The app uses environment variables `ACCOUNT_HOST` and `ACCOUNT_KEY`. Make sure these environment variables exist, and are set to your Azure Cosmos DB account `URI` and `PRIMARY KEY` respectively.

## Run application
1. git clone -b cosmos-spring-multi-tenant-samples https://github.com/TheovanKraay/azure-spring-boot-samples.git

Choose a reason for hiding this comment

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

Update this url

<dependency>
<groupId>com.azure</groupId>
<artifactId>azure-spring-data-cosmos</artifactId>
<version>LATEST</version>

Choose a reason for hiding this comment

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

Use property here.

<artifactId>multi-tenant-by-container</artifactId>
<version>1.0-SNAPSHOT</version>
<properties>
<azure.spring.data.cosmos.version>LATEST</azure.spring.data.cosmos.version>

Choose a reason for hiding this comment

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

Delete this property. Now the latest version is 3.33.0. And spring-cloud-azure-dependencies:4.7.0 already depends on azure-spring-data-cosmos:3.33.0.


@GetMapping(path = "/all")
public @ResponseBody String getAllTenantUsersAndOrders() {
//because order and user types are co-located with same partition key, we can hit the container once for both

Choose a reason for hiding this comment

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

To make the sample as simple as possible, how about just using one repository instead of 2 repositorires (UserRepository and OrderRepository) in this sample? (I remember I mentioned this before.)

Copy link
Contributor Author

@TheovanKraay TheovanKraay Apr 14, 2023

Choose a reason for hiding this comment

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

Although the structure of the entities themselves in the sample might justify this (because they are contrived and simple), in typical cases of co-locating the entities would probably need to be separate as they are too different. It is already a paradigm shift for user to consider co-locating, so prefer to keep this separate for now (same as reason for not using profiles). If we do anything, it will be to make the entities more complex in the future. But again, prefer to iterate on this (i.e. do this in subsequent PRs as the need arises).


@Repository
public interface OrderRepository extends CosmosRepository<Order, String> {
//get orders with custom query filtering by type since entities are co-located in same container

Choose a reason for hiding this comment

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

It will be easier if we do not bring co-located in this sample.

Copy link
Contributor Author

@TheovanKraay TheovanKraay Apr 14, 2023

Choose a reason for hiding this comment

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

Can you expand on why? The practice of co-locating entities is important and useful for Cosmos DB. This is the first time we are introducing it for Spring Data, because it is the first time it has been feasible (and it is a by-product of multi-tenant support at container level). It is also a differentiating feature.

Choose a reason for hiding this comment

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

@chenrujun - it is important to talk about co-location here, since that's how multi-tenancy will be achieved here.

@Query(value = "SELECT c.id, c.firstName, c.lastName, c.orderDetail FROM c")
Slice<JsonNode> getOrdersAndUsers(Pageable pageable);

//get orders with custom query filtering by type since entities are co-located in same container

Choose a reason for hiding this comment

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

Same here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As above.

<dependency>
<groupId>com.azure</groupId>
<artifactId>azure-spring-data-cosmos</artifactId>
<version>LATEST</version>

Choose a reason for hiding this comment

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

Same here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok

<artifactId>multi-tenant-by-database</artifactId>
<version>1.0-SNAPSHOT</version>
<properties>
<azure.spring.data.cosmos.version>LATEST</azure.spring.data.cosmos.version>

Choose a reason for hiding this comment

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

Same here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

import java.util.concurrent.ConcurrentHashMap;

@Component
@PropertySource("classpath:application.yaml")

Choose a reason for hiding this comment

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

Delete this if it's not necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

deleted

kushagraThapar
kushagraThapar previously approved these changes Apr 17, 2023
@@ -0,0 +1,11 @@
package com.azure.spring.data.cosmos.example;

Choose a reason for hiding this comment

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

missing license header

@@ -0,0 +1,30 @@
package com.azure.spring.data.cosmos.example;

Choose a reason for hiding this comment

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

missing license header

@@ -0,0 +1,65 @@
package com.azure.spring.data.cosmos.example;

Choose a reason for hiding this comment

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

missing license header


@Repository
public interface OrderRepository extends CosmosRepository<Order, String> {
//get orders with custom query filtering by type since entities are co-located in same container

Choose a reason for hiding this comment

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

@chenrujun - it is important to talk about co-location here, since that's how multi-tenancy will be achieved here.

@@ -0,0 +1,56 @@
package com.azure.spring.data.cosmos.example;

Choose a reason for hiding this comment

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

missing license header

@@ -0,0 +1,68 @@
package com.azure.spring.data.cosmos.example;

Choose a reason for hiding this comment

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

missing license header

@@ -0,0 +1,60 @@
package com.azure.spring.data.cosmos.example;

Choose a reason for hiding this comment

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

missing license header

@@ -0,0 +1,29 @@
package com.azure.spring.data.cosmos.example.tenant;

Choose a reason for hiding this comment

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

missing license header

@@ -0,0 +1,82 @@
package com.azure.spring.data.cosmos.example.tenant;

Choose a reason for hiding this comment

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

missing license header

@@ -0,0 +1,19 @@
package com.azure.spring.data.cosmos.example.tenant;

Choose a reason for hiding this comment

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

missing license header

@kushagraThapar kushagraThapar dismissed chenrujun’s stale review April 21, 2023 15:00

Reviewed the PR, need to get this out. Other improvements will come up in follow up PR

Copy link

@kushagraThapar kushagraThapar left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @TheovanKraay
@chenrujun - thanks for the review. Since this needs to get out, (and you are on vacation), let's add further improvements in a follow up PR, thanks!

@kushagraThapar kushagraThapar merged commit 2d7ca3e into Azure-Samples:main Apr 21, 2023
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.

4 participants