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

[#1967] Fetch available segements only from the TokenStore #1997

Merged
merged 18 commits into from Jan 3, 2022
Merged

[#1967] Fetch available segements only from the TokenStore #1997

merged 18 commits into from Jan 3, 2022

Conversation

Vermorkentech
Copy link
Contributor

@Vermorkentech Vermorkentech commented Nov 3, 2021 •
edited

This PR introduces a new method to the TokenStore; fetchAvailableSegments to fetch the ids of unclaimed segments only. This prevents streaming event processors from attempting to claim tokens that have already been claimed by another processor.

Additionally, both the TrackingEventProcessor and the PooledStreamingEventProccesor have been updated to utilize this method.

In order to preserve backwards compatibility, the fetchAvailableSegments method returns an Optional<int[]>, where an empty Optional should be interpreted as this method not being implemented.

UPDATE: Instead of returning an Optional<int[]>, I'm now returning a List<Segment>. This avoids the need to make another call to the DB later on to construct the segments. The default implementation simply returns all Segments, not only the available ones.
Additionally, instead of finding segments with a null owner, I'm now using the mayClaim method instead. This should now also include segments that have been claimed, but have not been updated in a while (possibly due to a node going down, etc.)

UPDATE 2: A potential race condition of claiming a token while performing a split or merge operation was detected. This already existed in the PooledStreamingEventProcessor and would have been introduced in this PR in the TrackingEventProcessor as well. As such, an update to the TokenStore has been added, which allows us to claim a segment by providing the entire Segment object, after which it will be validated. If not valid, it means a split or merge operation has just been performed concurrently and the segment's mask is no longer correct. In that case, the segment will be released again to be claimed at a later moment.

This PR resolves #1967

@Vermorkentech
Added a new fetchAvailableSegments method to the TokenStore, which on…
aee99f1 
…ly returns segments that have not yet been claimed.
@Vermorkentech
Modified Streaming event processors to now use TokenStore::fetchAvail…
5f81dd9 
…ableSegments instead of TokenStore::festSegments when getting new segments to claim.
@Vermorkentech Vermorkentech changed the title Feature/1967 [#1967] Fetch available segements only from the TokenStore Nov 3, 2021
@lfgcampos lfgcampos added Priority 3: Could Low priority. Issues that are nice to have but have a straightforward workaround. 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. labels Nov 3, 2021
@lfgcampos lfgcampos added this to the Release 4.6.0 milestone Nov 3, 2021
smcvb
smcvb approved these changes Nov 9, 2021
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 just got a couple of minor suggestions. Other than that, I think we've got an excellent addition here. Thank @Vermorkentech!

messaging/src/main/java/org/axonframework/eventhandling/TrackingEventProcessor.java Show resolved Hide resolved
messaging/src/main/java/org/axonframework/eventhandling/TrackingEventProcessor.java Outdated Show resolved Hide resolved
messaging/src/main/java/org/axonframework/eventhandling/tokenstore/TokenStore.java Outdated Show resolved Hide resolved
messaging/src/test/java/org/axonframework/eventhandling/tokenstore/jpa/JpaTokenStoreTest.java Show resolved Hide resolved
Vermorkentech and others added 2 commits November 9, 2021 19:46
@Vermorkentech @smcvb
Update javadoc messaging/src/main/java/org/axonframework/eventhandlin…
600cfa3 
…g/tokenstore/TokenStore.java

Co-authored-by: Steven van Beelen <steven.vanbeelen@axoniq.io>
@Vermorkentech
Added a test for TokenStore::fetchAvailableSegments default and fixed…
c9c6d0a 
… various review comments.
smcvb
smcvb approved these changes Nov 10, 2021
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 approving.

abuijze
abuijze requested changes Nov 19, 2021
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.

Discussed some API changes to make reduce the number of required storage-roundtrips while keeping backward compatibility.

smcvb
smcvb approved these changes Dec 1, 2021
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.

Except for some indentation nits, I think we're good to go.

@Vermorkentech
Moved fetching of segment into try-catch block
ae5aa71
@Vermorkentech
Added a new fecthToken method to the TokenStore that takes an entire …
b3989b6 
…Segment instead of the segmentId only. After fetching the token, some checks are performed to prevent a race condition with a concurrent split or merge of that segment.
@Vermorkentech
Merge branch 'master' into feature/1967
f22f5c7 
# Conflicts:
#	integrationtests/src/test/java/org/axonframework/integrationtests/eventhandling/TrackingEventProcessorTest.java
#	messaging/src/main/java/org/axonframework/eventhandling/TrackingEventProcessor.java
smcvb
smcvb requested changes Dec 30, 2021
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.

Just a bunch of minor improvements to cover left, I think.

messaging/src/main/java/org/axonframework/eventhandling/TrackingEventProcessor.java Outdated Show resolved Hide resolved
messaging/src/main/java/org/axonframework/eventhandling/tokenstore/jdbc/JdbcTokenStore.java Outdated Show resolved Hide resolved
messaging/src/main/java/org/axonframework/eventhandling/tokenstore/jdbc/JdbcTokenStore.java Outdated Show resolved Hide resolved
messaging/src/test/java/org/axonframework/eventhandling/tokenstore/jdbc/JdbcTokenStoreTest.java Show resolved Hide resolved
messaging/src/main/java/org/axonframework/eventhandling/tokenstore/jpa/JpaTokenStore.java Outdated Show resolved Hide resolved
messaging/src/main/java/org/axonframework/eventhandling/tokenstore/jpa/JpaTokenStore.java Outdated Show resolved Hide resolved
messaging/src/test/java/org/axonframework/eventhandling/tokenstore/jpa/JpaTokenStoreTest.java Show resolved Hide resolved
Vermorkentech and others added 5 commits December 30, 2021 22:03
@Vermorkentech @smcvb
Update messaging/src/main/java/org/axonframework/eventhandling/tokens…
569fcbe 
…tore/jdbc/JdbcTokenStore.java

Co-authored-by: Steven van Beelen <steven.vanbeelen@axoniq.io>
@Vermorkentech @smcvb
Update messaging/src/main/java/org/axonframework/eventhandling/tokens…
1ae8de8 
…tore/jdbc/JdbcTokenStore.java

Co-authored-by: Steven van Beelen <steven.vanbeelen@axoniq.io>
@Vermorkentech @smcvb
Update messaging/src/main/java/org/axonframework/eventhandling/tokens…
205401a 
…tore/jpa/JpaTokenStore.java

Co-authored-by: Steven van Beelen <steven.vanbeelen@axoniq.io>
@Vermorkentech
Calling the TokenStore in a transaction instead of directly
de445fc
@Vermorkentech
Merge branch 'master' into feature/1967
8ab50d6
smcvb
smcvb approved these changes Dec 31, 2021
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 approving.
I would still expect the deprecation notice on the TokenStore#fetchToken(String, int) method before merging as we've discussed, of course.

messaging/src/main/java/org/axonframework/eventhandling/TrackingEventProcessor.java Outdated Show resolved Hide resolved
messaging/src/main/java/org/axonframework/eventhandling/tokenstore/jpa/JpaTokenStore.java Outdated Show resolved Hide resolved
messaging/src/main/java/org/axonframework/eventhandling/tokenstore/jpa/JpaTokenStore.java Outdated Show resolved Hide resolved
messaging/src/test/java/org/axonframework/eventhandling/tokenstore/jdbc/JdbcTokenStoreTest.java Show resolved Hide resolved
Vermorkentech and others added 2 commits December 31, 2021 17:22
@Vermorkentech @smcvb
Update messaging/src/main/java/org/axonframework/eventhandling/tokens…
38b9ac0 
…tore/jpa/JpaTokenStore.java

Co-authored-by: Steven van Beelen <steven.vanbeelen@axoniq.io>
@Vermorkentech @smcvb
Update messaging/src/main/java/org/axonframework/eventhandling/Tracki…
3e0c340 
…ngEventProcessor.java

Co-authored-by: Steven van Beelen <steven.vanbeelen@axoniq.io>
@sonarcloud
Copy link

sonarcloud bot commented Dec 31, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 3 Code Smells

93.9% 93.9% Coverage
0.0% 0.0% Duplication

@Vermorkentech Vermorkentech merged commit 80e2033 into master Jan 3, 2022
@smcvb smcvb deleted the feature/1967 branch January 4, 2022 11:13
@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 Jan 4, 2022
@smcvb smcvb mentioned this pull request Jan 4, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abuijze abuijze abuijze approved these changes

@smcvb smcvb smcvb approved these changes

@lfgcampos lfgcampos Awaiting requested review from lfgcampos

@sandjelkovic sandjelkovic Awaiting requested review from sandjelkovic

Assignees

@Vermorkentech Vermorkentech

Labels
Priority 3: Could Low priority. Issues that are nice to have but have a straightforward workaround. 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
Release 4.6.0
Development

Successfully merging this pull request may close these issues.

Add method returning the available segments of a TokenStore
4 participants
@Vermorkentech @abuijze @smcvb @lfgcampos