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

fix: Don't trigger replay for duplicates #837

Merged
merged 1 commit into from
Apr 4, 2023

Conversation

patriknw
Copy link
Member

  • duplicates, less seqNr, have been processed and shouldn't trigger replay
  • need more details from R2dbcOffsetStore.isAccepted so changed return value to adt

* duplicates, less seqNr, have been processed and shouldn't trigger replay
* need more details from R2dbcOffsetStore.isAccepted so changed return value to adt
@patriknw patriknw force-pushed the wip-replay-duplicate-patriknw branch from 1eebb60 to 9df4661 Compare March 30, 2023 13:11
Copy link
Member

@octonato octonato left a comment

Choose a reason for hiding this comment

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

LGTM, as far I can tell.

Have some comments on naming though.

Comment on lines +173 to +177
object IsAcceptedResult {
case object Accepted extends IsAcceptedResult
case object Duplicate extends IsAcceptedResult
case object RejectedSeqNr extends IsAcceptedResult
case object RejectedBacktrackingSeqNr extends IsAcceptedResult
Copy link
Member

Choose a reason for hiding this comment

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

I find it confusing that the trait is called IsAcceptedResult, but it can be a rejection as well.

Maybe better to call is Validation instead?

  object Validation {
    case object Accepted extends Validation
    case object Duplicate extends Validation
    case object RejectedSeqNr extends Validation
    case object RejectedBacktrackingSeqNr extends Validation

Copy link
Member Author

Choose a reason for hiding this comment

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

alright, I'll follow up with some renaming in separate PR

@@ -656,14 +672,17 @@ private[projection] class R2dbcOffsetStore(
* Completed with `true` if accepted, otherwise `false` if rejected or failed with `EnvelopeRejected` if
* backtracking envelope is rejected.
*/
def isAccepted[Envelope](envelope: Envelope): Future[Boolean] = {
def isAccepted[Envelope](envelope: Envelope): Future[IsAcceptedResult] = {
Copy link
Member

Choose a reason for hiding this comment

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

Following previous comment...

  def validate[Envelope](envelope: Envelope): Future[Validation]

Also, scaladoc needs to be adapted. Not a boolean anymore.

@patriknw
Copy link
Member Author

patriknw commented Apr 4, 2023

test failures are in unrelated modules

@patriknw patriknw merged commit a081fc9 into dev-filter Apr 4, 2023
@patriknw patriknw deleted the wip-replay-duplicate-patriknw branch April 4, 2023 09:15
Copy link
Member

@johanandren johanandren left a comment

Choose a reason for hiding this comment

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

After the fact LGTM

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