-
Notifications
You must be signed in to change notification settings - Fork 66
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
if results too large get each result one at a time #1118
Conversation
…ach transaction result at a time and append it to the array
Codecov Report
@@ Coverage Diff @@
## master #1118 +/- ##
=======================================
Coverage 39.28% 39.28%
=======================================
Files 37 37
Lines 1876 1876
=======================================
Hits 737 737
Misses 1051 1051
Partials 88 88
Flags with carried forward coverage won't be shown. Click here to find out more. |
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, just one minor comment
…: code = ResourceExhausted desc = grpc: trying to send message larger than max (26319669 vs. 20971520)
Question: will this not hit rate limit really fast? The blocks where this hits have hundreds of transactions and that will result in a lot of hits against the grpc endpoint. |
The gateway should take care of handling rate limits I think. So basically outgoing calls would automatically be delayed if they are getting close to the rate limit. I did something like that here previously https://github.com/onflow/flow-batch-scan/blob/main/client/interceptors/uci_rate_limit.go |
…e, for instance block 55179362
I just figured out that this will not work for some scenarios. Basically the only way atm to get SystemChunkTransactionResults is through the method that hits the limits. My last commit could possible be ignored since i was confused by the changed systemChunkTransactionId |
Block id 55179362 is a block that has systemChunkTransactions and it is not possible to get all transactionResults for that block since it is too large, how do i then get the systemChunkEvents? |
Ammended the pr with using the new GetTransactionByHeight that i exposed in another PR. |
} | ||
errorMessage := err.Error() | ||
|
||
if strings.Contains(errorMessage, "received message larger than max") || strings.Contains(errorMessage, "trying to send message larger than max") { |
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.
do we have access to these errors? so we could import them by type instead of hardcoding error message?
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.
they look like standard GRPC error messages to me, so i doubt 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.
Good idea to handle this, left some comments
I pushed changes but they do not appear here |
We have decided to not do this, it should be done on another level. |
Could not find a test for the original method so i did not add one here.
Closes #1107