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

[Access] Cache Transaction Result error messages #5066

Conversation

Guitarheroua
Copy link
Contributor

#4850

This PR introduces a straightforward pipeline designed to retrieve error messages from ENs (Execution Nodes). The pipeline is constructed using higher-level methods, initially checking the cache for relevant information. If the required value is absent in the cache, the pipeline employs specific methods to perform robust RPC requests to ENs for fetching missing values.

As this feature is intended to integrate with other functionalities related to indexing execution data, there haven't been modifications made to the external API, as it is expected to be paired with storage.LightTransactionResults to facilitate the implementation of the GetTransactionResult method.

All newly added methods have been covered by unit testing, even though they are not part of the public API interface.

@codecov-commenter
Copy link

codecov-commenter commented Nov 27, 2023

Codecov Report

Attention: 36 lines in your changes are missing coverage. Please review.

Comparison is base (87380a4) 56.25% compared to head (a1d568e) 56.28%.

Files Patch % Lines
engine/access/rpc/backend/backend_transactions.go 83.74% 24 Missing and 9 partials ⚠️
engine/access/rpc/backend/backend.go 66.66% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5066      +/-   ##
==========================================
+ Coverage   56.25%   56.28%   +0.03%     
==========================================
  Files         977      977              
  Lines       91472    91684     +212     
==========================================
+ Hits        51456    51603     +147     
- Misses      36202    36258      +56     
- Partials     3814     3823       +9     
Flag Coverage Δ
unittests 56.28% <83.01%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@peterargue peterargue left a comment

Choose a reason for hiding this comment

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

thanks @Guitarheroua! added a few comments.

}

// nothing to do tx has not failed
if !txResult.Failed {
Copy link
Contributor

Choose a reason for hiding this comment

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

in practice, lookupTransactionErrorMessage will only be called after looking up the result and checking that it failed. You can remove that part from these methods

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, that sounds good. I've removed it from lookupTransactionErrorMessage but maintained similar calls to storage.LightTransactionResults in *byIndex and *byBlockID calls. The reason for this is that the cache stores data using txid, and I require a means to look it up. I don't anticipate it causing significant overhead since the storage layer has its own cache, and it's still more cost-effective than initiating a GRPC call.

For the next iteration, we could even consider passing lightTransactionResult as an argument if we're going to query it anyway. Therefore, my suggestion is to leave it in this manner for the time being.

engine/access/rpc/backend/backend_transactions.go Outdated Show resolved Hide resolved
engine/access/rpc/backend/backend_transactions.go Outdated Show resolved Hide resolved
engine/access/rpc/backend/backend_transactions.go Outdated Show resolved Hide resolved
engine/access/rpc/backend/backend_transactions.go Outdated Show resolved Hide resolved
engine/access/rpc/backend/backend_transactions.go Outdated Show resolved Hide resolved
Copy link
Contributor

@peterargue peterargue left a comment

Choose a reason for hiding this comment

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

thanks for adding this! added a few small comments, but otherwise looks good

engine/access/rpc/backend/backend.go Outdated Show resolved Hide resolved
engine/access/rpc/backend/backend_transactions_test.go Outdated Show resolved Hide resolved
engine/access/rpc/backend/backend_transactions_test.go Outdated Show resolved Hide resolved
Copy link
Member

@durkmurder durkmurder left a comment

Choose a reason for hiding this comment

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

Looks good

@durkmurder durkmurder added this pull request to the merge queue Dec 4, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 4, 2023
@durkmurder durkmurder added this pull request to the merge queue Dec 4, 2023
Merged via the queue into onflow:master with commit 0c0a54d Dec 4, 2023
54 checks passed
peterargue pushed a commit that referenced this pull request Dec 12, 2023
…-result-err-msg

[Access] Cache Transaction Result error messages
peterargue pushed a commit that referenced this pull request Dec 12, 2023
…-result-err-msg

[Access] Cache Transaction Result error messages
peterargue pushed a commit that referenced this pull request Dec 13, 2023
…-result-err-msg

[Access] Cache Transaction Result error messages
@Guitarheroua Guitarheroua deleted the guitarheroua/4850-cache-tx-result-err-msg branch December 27, 2023 11: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.

4 participants