-
Notifications
You must be signed in to change notification settings - Fork 33
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
fix: Start from earliest slice for downscaling scenario #1012
Conversation
* start from earliest slice when projection key is changed
30150fc
to
93fe817
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Earliest offset will always cover it, may just be further back than necessary.
Test environment no longer exists — should we set up a new one for testing these changes?
akka-projection-r2dbc/src/it/scala/akka/projection/r2dbc/ChangeSliceRangesSpec.scala
Outdated
Show resolved
Hide resolved
// offset of the earliest slice | ||
val latestBySlice = newState.latestBySlice | ||
val earliest = latestBySlice.minBy(_.timestamp).timestamp | ||
Some(TimestampOffset(earliest, Map.empty)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the latest offset, the seen map is populated. When I was experimenting with the earliest or latest-by-split offsets, I also ended up creating the seen map from byPid
for the selected offset here. Shouldn't affect behaviour when eventually deduplicated, but I think at least the metrics were confused (inactive projections had growing consumer lag metrics).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's important to reconstruct it in this case, but I added the obvious from earliest
record. 4ab18c3
Co-authored-by: Peter Vlugter <59895+pvlugter@users.noreply.github.com>
@pvlugter @johanandren Shall we go with this? So that we include it in our testing? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to have it in testing. And I'll look to recreate the bigger tests that we've been running before final release.
New iteration of #995 and #1004