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.

Sign up for GitHub

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

Jump to bottom

Add a TokenStore implementation for CosmosDB #1

Merged
merged 11 commits into from
Feb 2, 2024
Merged

Add a TokenStore implementation for CosmosDB #1

merged 11 commits into from
Feb 2, 2024

Conversation

gklijs
Copy link
Contributor

@gklijs gklijs commented Jan 8, 2024 •
edited
Loading

It seems like for the integration tests, we are blocked by Azure/azure-cosmos-db-emulator-docker#56. I also don't want to use an unsupported ubuntu.. Maybe put the integration test behind a profile, so the tests do complete for now?

@gklijs gklijs added the Priority 2: Should High priority. Ideally, these issues are part of the release they’re assigned to. label Jan 8, 2024
@gklijs gklijs self-assigned this Jan 8, 2024
@smcvb smcvb added Priority 1: Must Highest priority. A release cannot be made if this issue isn’t resolved. Status: In Progress Use to signal this issue is actively worked on. Type: Feature Use to signal an issue is completely new to the project. and removed Priority 2: Should High priority. Ideally, these issues are part of the release they’re assigned to. labels Jan 9, 2024
@smcvb smcvb changed the title Add the token store implementation for Cosmos. Add a TokenStore implementation for CosmosDB Jan 9, 2024
smcvb
smcvb requested changes Jan 10, 2024
View reviewed changes
Copy link
Member

@smcvb smcvb left a comment

Choose a reason for hiding this comment

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

My comments can be broken down in roughly two groups:

  1. Project basis missers. As stated in a comment, would've been nice to do these in a separate PR.
  2. Questions and concerns around the CosmosTokenStore.

.gitattributes Show resolved Hide resolved
.github/ISSUE_TEMPLATE/1_feature_request.md Outdated Show resolved Hide resolved
.github/ISSUE_TEMPLATE/1_feature_request.md Outdated Show resolved Hide resolved
.github/ISSUE_TEMPLATE/2_enhancement_request.md Outdated Show resolved Hide resolved
.github/ISSUE_TEMPLATE/2_enhancement_request.md Outdated Show resolved Hide resolved
...in/java/org/axonframework/extensions/cosmosdb/eventsourcing/tokenstore/CosmosTokenStore.java Outdated Show resolved Hide resolved
...in/java/org/axonframework/extensions/cosmosdb/eventsourcing/tokenstore/CosmosTokenStore.java Outdated Show resolved Hide resolved
...nframework/extensions/cosmosdb/eventsourcing/tokenstore/CosmosTokenStoreIntegrationTest.java Show resolved Hide resolved
...nframework/extensions/cosmosdb/eventsourcing/tokenstore/CosmosTokenStoreIntegrationTest.java Outdated Show resolved Hide resolved
...nframework/extensions/cosmosdb/eventsourcing/tokenstore/CosmosTokenStoreIntegrationTest.java Outdated Show resolved Hide resolved
@CLAassistant
Copy link

CLAassistant commented Jan 10, 2024 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@gklijs gklijs force-pushed the feature/token-store branch from 5636c80 to 9c4e757 Compare January 10, 2024 12:11
@gklijs gklijs requested a review from smcvb January 10, 2024 12:12
@gklijs
Copy link
Contributor Author

gklijs commented Jan 10, 2024

I addressed most points, but two are still open.

@smcvb smcvb mentioned this pull request Jan 11, 2024
Merged
smcvb
smcvb reviewed Jan 16, 2024
View reviewed changes
Copy link
Member

@smcvb smcvb left a comment

Choose a reason for hiding this comment

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

Another, yet smaller, bunch of comments. It's definitely shaping up 👍

...in/java/org/axonframework/extensions/cosmosdb/eventsourcing/tokenstore/CosmosTokenStore.java Outdated Show resolved Hide resolved
...in/java/org/axonframework/extensions/cosmosdb/eventsourcing/tokenstore/CosmosTokenStore.java Outdated Show resolved Hide resolved
...in/java/org/axonframework/extensions/cosmosdb/eventsourcing/tokenstore/CosmosTokenStore.java Outdated Show resolved Hide resolved
...ain/java/org/axonframework/extensions/cosmosdb/eventsourcing/tokenstore/CosmosTokenItem.java Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
.../axonframework/extensions/cosmosdb/eventsourcing/tokenstore/CosmosTokenStoreBuilderTest.java Show resolved Hide resolved
...in/java/org/axonframework/extensions/cosmosdb/eventsourcing/tokenstore/CosmosTokenStore.java Outdated Show resolved Hide resolved
...in/java/org/axonframework/extensions/cosmosdb/eventsourcing/tokenstore/CosmosTokenStore.java Show resolved Hide resolved
...in/java/org/axonframework/extensions/cosmosdb/eventsourcing/tokenstore/CosmosTokenStore.java Outdated Show resolved Hide resolved
...nframework/extensions/cosmosdb/eventsourcing/tokenstore/CosmosTokenStoreIntegrationTest.java Show resolved Hide resolved
gklijs and others added 2 commits January 19, 2024 10:32
@gklijs @smcvb
Update README.md
1fb1c2b 
Co-authored-by: Steven van Beelen <[email protected]>
Make the tokenstore use a specific container instead of using a diffe…
c1596bd 
…rent container for each processing group.
@gklijs gklijs requested a review from smcvb January 19, 2024 11:27
@gklijs
Copy link
Contributor Author

gklijs commented Jan 19, 2024

Changed the implementation so one token store instance is only using one container, and thus uses a query for the cases where it needs to list for processor name.

smcvb
smcvb reviewed Feb 1, 2024
View reviewed changes
Copy link
Member

@smcvb smcvb left a comment

Choose a reason for hiding this comment

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

Some very minor things remaining!

...ain/java/org/axonframework/extensions/cosmosdb/eventsourcing/tokenstore/CosmosTokenItem.java Outdated Show resolved Hide resolved
...ain/java/org/axonframework/extensions/cosmosdb/eventsourcing/tokenstore/CosmosTokenItem.java Outdated Show resolved Hide resolved
...ain/java/org/axonframework/extensions/cosmosdb/eventsourcing/tokenstore/CosmosTokenItem.java Show resolved Hide resolved
...in/java/org/axonframework/extensions/cosmosdb/eventsourcing/tokenstore/CosmosTokenStore.java Outdated Show resolved Hide resolved
...ain/java/org/axonframework/extensions/cosmosdb/eventsourcing/tokenstore/CosmosTokenItem.java Outdated Show resolved Hide resolved
@gklijs gklijs requested a review from smcvb February 2, 2024 08:28
@sonarqubecloud SonarQubeCloud
Copy link

sonarqubecloud bot commented Feb 2, 2024

Quality Gate Failed Quality Gate failed

Failed conditions

10.2% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

smcvb
smcvb approved these changes Feb 2, 2024
View reviewed changes
Copy link
Member

@smcvb smcvb left a comment

Choose a reason for hiding this comment

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

All my concerns have been addressed, hence approving!

@gklijs gklijs merged commit cfe823e into main Feb 2, 2024
6 of 7 checks passed
@gklijs gklijs deleted the feature/token-store branch February 2, 2024 09:14
@smcvb smcvb added Status: Resolved Use to signal that work on this issue is done. and removed Status: In Progress Use to signal this issue is actively worked on. labels Feb 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smcvb smcvb smcvb approved these changes

@CodeDrivenMitch CodeDrivenMitch Awaiting requested review from CodeDrivenMitch

Assignees

@gklijs gklijs

Labels
Priority 1: Must Highest priority. A release cannot be made if this issue isn’t resolved. Status: Resolved Use to signal that work on this issue is done. Type: Feature Use to signal an issue is completely new to the project.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gklijs @CLAassistant @smcvb