-
Notifications
You must be signed in to change notification settings - Fork 6
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 read records #132
Fix read records #132
Conversation
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.
LGTM! Thank you!
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.
Left a minor comment. Other than that, LGTM! Thank you!
(range n)))] | ||
(.commit tx) | ||
results) | ||
(read-record test id) | ||
(catch Exception e |
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 think we can catch CrudConflictException
if it's for the lazy recovery. What do you think?
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.
@brfrn169 Another exception could be thrown from the commit, couldn't it?
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.
@yito88 Sorry, it seems I had the wrong idea. Please ignore the comment.
So currently, we retry all the record reads together, but after this PR, we will retry each record read individually. Is my understanding correct?
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.
@brfrn169 No problem. "read-record" tries to update, so it's a bit misleading.
we retry all the record reads together, but after this PR, we will retry each record read individually. Is my understanding correct?
Exactly!
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.
Thanks!
Description
Try to read each account with retries.
Basically, the backoff retry is to wait for the DB bootstrapping after the nemesis, not to recover these records. When some records needed to be recovered, e.g. all 10 records, the test failed because the retry count (8) was fixed.
Related issues and/or PRs
Changes made
read-all
to read each account with retries.Checklist
Additional notes (optional)