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

Persistent streams #206

Merged
merged 18 commits into from
Jul 23, 2024
Merged

Persistent streams #206

merged 18 commits into from
Jul 23, 2024

Conversation

schananas
Copy link
Contributor

Implementation and support for persistent streams

@schananas schananas changed the base branch from main to axon-multitenancy-4.9.x July 8, 2024 11:00
@schananas schananas changed the base branch from axon-multitenancy-4.9.x to main July 8, 2024 11:02
@schananas schananas changed the base branch from main to axon-multitenancy-4.9.x July 8, 2024 11:03
@schananas schananas requested review from abuijze and smcvb July 8, 2024 11:13
@smcvb smcvb added this to the Release 4.10.0 milestone Jul 18, 2024
@smcvb
Copy link
Member

smcvb commented Jul 18, 2024

Since this feature is intended to work in combination with Axon Framework 4.10.0, the PR should also be part of Multitenancy extension 4.10.0.
To that end, we should be using the main branch instead of patch branch axon-multitenancy-4.9.x, as 4.10.0 is the next minor release.
So, would you mind switching the base branch from 4.9.x to main?
By doing so, we also achieve the option to add documentations right away too, since the new documentation approach is place in main already.

@schananas schananas changed the base branch from axon-multitenancy-4.9.x to main July 18, 2024 10:41
@schananas
Copy link
Contributor Author

Since this feature is intended to work in combination with Axon Framework 4.10.0, the PR should also be part of Multitenancy extension 4.10.0. To that end, we should be using the main branch instead of patch branch axon-multitenancy-4.9.x, as 4.10.0 is the next minor release. So, would you mind switching the base branch from 4.9.x to main? By doing so, we also achieve the option to add documentations right away too, since the new documentation approach is place in main already.

done

abuijze
abuijze requested changes Jul 22, 2024
View reviewed changes
Copy link
Member

@abuijze abuijze left a comment

Choose a reason for hiding this comment

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

There is some markdown in between javadoc entries and some unused imports in classes.

...framework/extensions/multitenancy/autoconfig/TenantPersistentStreamMessageSourceFactory.java Outdated Show resolved Hide resolved
...ain/java/org/axonframework/extensions/multitenancy/components/MultiTenantAwareComponent.java Outdated Show resolved Hide resolved
@schananas
PR comments
25c10a0
@schananas
Merge branch 'main' into feature/streams
9e7b710 
# Conflicts:
#	multitenancy-spring-boot-3-integrationtests/pom.xml
#	pom.xml
@schananas schananas requested a review from abuijze July 22, 2024 09:01
smcvb
smcvb requested changes Jul 23, 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.

I think the implementation is in line with what I expected, but there are quite some clean-up things I believe we should cover before we start merging this.

Nonetheless, great job, and thanks for the PR! 🙌

multitenancy-spring-boot-3-integrationtests/pom.xml Outdated Show resolved Hide resolved
...onframework/extensions/multitenancy/autoconfig/MultiTenantPersistentStreamMessageSource.java Show resolved Hide resolved
...onframework/extensions/multitenancy/autoconfig/MultiTenantPersistentStreamMessageSource.java Show resolved Hide resolved
...onframework/extensions/multitenancy/autoconfig/MultiTenantPersistentStreamMessageSource.java Show resolved Hide resolved
...onframework/extensions/multitenancy/autoconfig/MultiTenantPersistentStreamMessageSource.java Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
...rg/axonframework/extensions/multitenancy/configuration/MultiTenantEventProcessingModule.java Outdated Show resolved Hide resolved
...amework/extensions/multitenancy/autoconfig/MultiTenantPersistentStreamAutoConfiguration.java Show resolved Hide resolved
...amework/extensions/multitenancy/autoconfig/MultiTenantPersistentStreamMessageSourceTest.java Outdated Show resolved Hide resolved
...onframework/extensions/multitenancy/autoconfig/MultiTenantPersistentStreamMessageSource.java Show resolved Hide resolved
schananas and others added 3 commits July 23, 2024 11:47
@schananas
PR comments
68d184b
@schananas @smcvb
Update multitenancy/src/main/java/org/axonframework/extensions/multit…
32000ab 
…enancy/configuration/MultiTenantEventProcessingModule.java

Co-authored-by: Steven van Beelen <steven.vanbeelen@axoniq.io>
@schananas @smcvb
Update multitenancy/src/main/java/org/axonframework/extensions/multit…
9ac4b3f 
…enancy/components/MultiTenantAwareComponent.java

Co-authored-by: Steven van Beelen <steven.vanbeelen@axoniq.io>
@schananas schananas requested a review from smcvb July 23, 2024 09:51
smcvb
smcvb reviewed Jul 23, 2024
View reviewed changes
multitenancy/pom.xml
@@ -62,5 +62,9 @@
<version>${mockito.version}</version>
<scope>test</scope>
</dependency>
<dependency>
Copy link
Member

Choose a reason for hiding this comment

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

Ah, bummer, I didn't think about the dependency that would move if we'd do this...
Let's move the Persistent Stream implementing class back to the auto-configuration module.

My apologies for sending you back and forth!

@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Jul 23, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
66.7% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

smcvb
smcvb approved these changes Jul 23, 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 concerns have been addressed, hence I'm approving this pull request.

@smcvb smcvb merged commit 4b7d521 into main Jul 23, 2024
6 of 7 checks passed
@smcvb smcvb deleted the feature/streams branch July 23, 2024 15:31
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

@abuijze abuijze Awaiting requested review from abuijze

Assignees

@schananas schananas

Labels
Priority 1: Must Type: Feature
Projects
None yet
Milestone
Release 4.10.0
Development

Successfully merging this pull request may close these issues.
3 participants
@schananas @smcvb @abuijze