Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: Count S3 get requests made by
near-lake-framework
#662feat: Count S3 get requests made by
near-lake-framework
#662Changes from 4 commits
ad8f20d
bab4627
f42f843
e30878d
bd3d75c
1ffbd48
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
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.
Is this call out an indication that we need to rethink the function declaration? Either encapsulate some or all of these into some BlockStreamContext, or roll some of these into each other (redis stream does into IndexerConfig, Lake Framework stream created before being passed into start_block_stream, etc)?
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.
Yeah - this suppresses the error which I was getting sick of.
BlockStreamContext
is a good idea.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.
Will try to address this in a future PR, will probably need to add a config option to Near Lake which prevents infinite retries.
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.
What change to Lake caused infinite retries?
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.
What is this function used for? Just testing or something else?
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.
This is part of the
S3Client
trait exposed bynear_lake_framework
. Under the hood it's used to list block heights, and was originally added to mock those heights returned. We don't do anything with it now, but still need to supply it.We may want to add caching here too, but it's a bit more complicated because the arguments will likely vary widely across all
near_lake_framework
instances.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.
Is this a value we set somewhere previously or was 1000 the default by the API?
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.
This is the default from the API, essentially just a copy paste from
near_lake_framework
code.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.
Unable to use usual
#[automock]
due to the multipleimpl
blocks and trait implementation, so have to use this custommock!
block. The end result is essentially the same.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.
Latest
near-lake-framework
changed these toAccountId
fromString