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

Prevent nil receipts and added retry logic for making chain events #977

Closed

Conversation

jeongkyun-oh
Copy link
Contributor

@jeongkyun-oh jeongkyun-oh commented May 26, 2021

Proposed changes

Making a chain event is done locally after getting items from database. It was hard to get an error, but the error case is not handled properly. This change checks if receipt list is nil or not, and added retrying logic for making chain events.

Unlike retrying to produce Kafka messages, it should not fail to get data from the database. It retries inifinately, but after 100 retry it leaves error logs.

Types of changes

Please put an x in the boxes related to your change.

  • Bugfix
  • New feature or enhancement
  • Others

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

  • I have read the CONTRIBUTING GUIDELINES doc
  • I have signed the CLA
  • Lint and unit tests pass locally with my changes ($ make test)
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • Any dependent changes have been merged and published in downstream modules

@jeongkyun-oh jeongkyun-oh self-assigned this May 26, 2021
@jeongkyun-oh jeongkyun-oh changed the title Klt 861 update make chain event Prevent nil receipts and added retry logic for making chain events May 26, 2021
hyochan-brown
hyochan-brown previously approved these changes May 27, 2021
Copy link
Contributor

@hyochan-brown hyochan-brown left a comment

Choose a reason for hiding this comment

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

LGTM!!
I have only one question.

datasync/chaindatafetcher/chaindata_fetcher.go Outdated Show resolved Hide resolved
aidan-kwon
aidan-kwon previously approved these changes May 27, 2021
ehnuje
ehnuje previously approved these changes May 27, 2021
@jeongkyun-oh jeongkyun-oh dismissed stale reviews from ehnuje and aidan-kwon via c8c6f32 May 27, 2021 11:51
aidan-kwon
aidan-kwon previously approved these changes May 28, 2021
ehnuje
ehnuje previously approved these changes May 28, 2021
@jeongkyun-oh jeongkyun-oh dismissed stale reviews from ehnuje and aidan-kwon via 42dbd42 May 28, 2021 05:58
@jeongkyun-oh jeongkyun-oh force-pushed the KLT-861-update-make-chain-event branch from 42dbd42 to d13cb3a Compare May 28, 2021 06:00
@jeongkyun-oh jeongkyun-oh force-pushed the KLT-861-update-make-chain-event branch from d13cb3a to 794d477 Compare May 28, 2021 06:36
@KimKyungup
Copy link
Contributor

@jeongkyun-oh What is the root cause of not having a receipt?

@jeongkyun-oh
Copy link
Contributor Author

@KimKyungup Unexpected errors occur in DynamoDB unlike LevelDB. The retry logic is added for unexpected temporary errors.

@jeongkyun-oh
Copy link
Contributor Author

Dear reviewers, sorry for confusion.

For now, #982 PR fixes the current issue.
For retry, I will make another PR in the near future.
I made another PR for checking nil receipts in #983.

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.

None yet

6 participants