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

feat: Handle heartbeat events #1200

Closed
wants to merge 3 commits into from
Closed

feat: Handle heartbeat events #1200

wants to merge 3 commits into from

Conversation

patriknw
Copy link
Member

@patriknw
Copy link
Member Author

patriknw commented Sep 19, 2024

R2dbcOffsetStore should store these heartbeat offsets, and it is storing the first one, but since the seqNr is always 1 it will not store any updates (duplicates). Probably need special case for those in the offset store to bypass the validation and increment the seqNr before storing. Hm, I'll think some more about that.

fixed: 908cfa1

Later we probably want to store offset progress for each slice from the heartbeats and that will also require special case in the offset store.

TimestampOffset(startTime.plus(evictInterval).plus(JDuration.ofSeconds(1)), Map(p4 -> 1L)),
p4,
1L))
.futureValue
Copy link
Member

Choose a reason for hiding this comment

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

These are now no longer evicted because of the keep latest change?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure what the original intention with this one was, but it was never saved because it was a duplicate. Same p4 and seqNr 1 as above. I changed isDuplicate to also look at timestamp when same seqNr and then this test failed. By removing this the test is kind of as it was before.

TimestampOffset(startTime.plus(evictInterval).plus(JDuration.ofSeconds(1)), Map(p4 -> 1L)),
p4,
1L))
.futureValue
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure what the original intention with this one was, but it was never saved because it was a duplicate. Same p4 and seqNr 1 as above. I changed isDuplicate to also look at timestamp when same seqNr and then this test failed. By removing this the test is kind of as it was before.

TimestampOffset(startTime.plus(evictInterval).plus(JDuration.ofSeconds(1)), Map(p4 -> 1L)),
p4,
1L))
.futureValue
Copy link
Member Author

Choose a reason for hiding this comment

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

and same here

Copy link
Contributor

@pvlugter pvlugter left a comment

Choose a reason for hiding this comment

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

Looks good

@patriknw patriknw force-pushed the wip-heartbeat-patriknw branch from 908cfa1 to 32db079 Compare October 9, 2024 15:01
@patriknw
Copy link
Member Author

We decided to not emit the heartbeats after the query subscriber, so they will not reach the offset store.

@patriknw patriknw closed this Oct 18, 2024
@patriknw patriknw deleted the wip-heartbeat-patriknw branch October 18, 2024 09:40
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.

3 participants