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

Balance monitoring retries #82

Merged
merged 4 commits into from
Sep 2, 2021
Merged

Balance monitoring retries #82

merged 4 commits into from
Sep 2, 2021

Conversation

nkuba
Copy link
Member

@nkuba nkuba commented Sep 1, 2021

We want to use a retry mechanism for single executions of the balance check.
The retries will be performed during the period of retryTimeout, logging a warning on each error. When the timeout is hit an error will be logged and retries stopped. The next balance check will be triggered at the next tick.

@nkuba nkuba requested a review from pdyraga September 1, 2021 12:42
We want to use a retry mechanism for single executions of the balance check.
The retries will be performed during the period of `retryTimeout`,
logging a warning on each error. When the timeout is hit an error
will be logged and retries stopped. A next balance check will be triggered at the tick.
@nkuba nkuba force-pushed the balance-monitoring-retries branch from 051d018 to 26fdfa2 Compare September 1, 2021 12:45
We added a check execution at start of the monitoring. Previously we had
to wait for ticker to invoke the first check.
The test checks calls to the balance source function. In the future we
should check logged messages.
func (bm *BalanceMonitor) Observe(
ctx context.Context,
address Address,
alertThreshold *Token,
tick time.Duration,
retryTimeout time.Duration,
Copy link
Member

Choose a reason for hiding this comment

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

Non-blocking but shouldn't we validate retryTimeout < tick so that we do not have two checks overlapping?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, good idea.

Copy link
Member Author

@nkuba nkuba Sep 2, 2021

Choose a reason for hiding this comment

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

On the other hand, don't we want to leave it to the implementation using this function?

@pdyraga pdyraga merged commit adb7bbc into main Sep 2, 2021
@pdyraga pdyraga deleted the balance-monitoring-retries branch September 2, 2021 16:44
@pdyraga pdyraga added this to the v1.7.0 milestone Sep 3, 2021
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