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

fixing a bug in which all handler retires executed within the same #82

Merged
merged 1 commit into from
Jun 5, 2019

Conversation

rhinof
Copy link
Contributor

@rhinof rhinof commented Jun 5, 2019

transaction

When a handler fails and gets retried each retry should be run in a new
transaction

Closes #81

transaction

When a handler fails and gets retried each retry should be run in a new
transaction
@rhinof rhinof requested review from vladshub, avigailberger and a user June 5, 2019 03:19
Copy link
Contributor

@vladshub vladshub left a comment

Choose a reason for hiding this comment

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

Where are the tests proving this issue?

_ = worker.ack(delivery)
}
//else there was an error in the invokation then try rollingback the transaction and reject the message
_ = worker.ack(delivery)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you ignoring the error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

after examining the code of worker.ack (and worker.nack) the retry strategy defined there retries the action until it succeeds (returns no error) in 100ms intervals

Copy link
Contributor

Choose a reason for hiding this comment

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

then we should fix the function . not to return an error but it would be a whole different PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, will do it in a diff PR

worker.log().WithError(rbkErr).Error("failed rolling back transaction when recovering from handler error")
}
}
return handlerErr
Copy link
Contributor

Choose a reason for hiding this comment

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

🏆

@vladshub
Copy link
Contributor

vladshub commented Jun 5, 2019

image

@rhinof rhinof merged commit 382bf3f into v1.x Jun 5, 2019
@rhinof rhinof deleted the fixtx branch June 5, 2019 06:19
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.

2 participants