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

Retry mismatched Rpc requests with error response #804

Merged
merged 5 commits into from
Nov 13, 2023

Conversation

cabrador
Copy link
Collaborator

@cabrador cabrador commented Nov 8, 2023

Description

This PR adds feature that retries any missmatch in the Rpc. This only works for records with error response. Valid response does not make sense to replay.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

@cabrador cabrador marked this pull request as ready for review November 8, 2023 12:27
c.log.Debugf("current ration retried against total %v/%v", c.numberOfRetriedRequests, c.totalNumberOfRequests)
c.numberOfRetriedRequests++
state.Data.StateDB.IsRecovered = true
compareErr = retryRequest(state)
Copy link
Collaborator

Choose a reason for hiding this comment

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

If I understand it correctly. If RPC request fails(/result is different), then the code attempts just 1 more time the same request? Wouldn't multiple retries be useful?

Copy link
Collaborator

Choose a reason for hiding this comment

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

My stance on this is to have our own opera archive running. Having multiple attempts doesn't guarantee stable result. If an error comes from the recording, a single retry connecting to a stable opera client should produce a correct result.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If I understand it correctly. If RPC request fails(/result is different), then the code attempts just 1 more time the same request? Wouldn't multiple retries be useful?

Yes and no. If comparison fails, request is send to Rpc Gateway to obtain new response. Then it is compared again. From what I have tried, this has 100% recovery rate.

@@ -108,6 +140,11 @@ func (c *rpcComparator) PostTransaction(state executor.State[*rpc.RequestAndResu

// PostRun prints all caught errors.
func (c *rpcComparator) PostRun(executor.State[*rpc.RequestAndResults], *executor.Context, error) error {
// log only if continue on failure is enabled
Copy link
Collaborator

Choose a reason for hiding this comment

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

I presume if error occurred and it got raised in Process then the PostRun isn't called at all. If so, does it even make call then to check in here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is no way Process causes error. It can possibly cause a panic and in that case this would not be called because of how executor handles panics.

@cabrador cabrador mentioned this pull request Nov 9, 2023
2 tasks
executor/extension/validator/rpc_comparator.go Outdated Show resolved Hide resolved
executor/extension/validator/rpc_comparator.go Outdated Show resolved Hide resolved
executor/extension/validator/rpc_comparator.go Outdated Show resolved Hide resolved
@cabrador cabrador force-pushed the petr/retry-rpc-requests branch from 82205ac to 13257dd Compare November 10, 2023 08:53
@cabrador cabrador merged commit 493c14e into develop Nov 13, 2023
1 check passed
@cabrador cabrador deleted the petr/retry-rpc-requests branch November 13, 2023 08:41
cabrador added a commit that referenced this pull request Nov 13, 2023
@cabrador cabrador restored the petr/retry-rpc-requests branch November 13, 2023 09:10
@wsodsong wsodsong deleted the petr/retry-rpc-requests branch April 18, 2024 09:10
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