feat: Count S3 get requests made by near-lake-framework
#662
Conversation
morgsmccauley
force-pushed
the
feat/count-lake-requests
branch
from
69a1434 to
e30878d
Compare
|
|
||
| // FIX: near lake framework now infinitely retires - we need a way to stop it to allow the test |
There was a problem hiding this comment.
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.
What change to Lake caused infinite retries?
| } | ||
|
|
||
| #[cfg(test)] | ||
| mockall::mock! { |
There was a problem hiding this comment.
Unable to use usual #[automock] due to the multiple impl blocks and trait implementation, so have to use this custom mock! block. The end result is essentially the same.
| || wildmatch::WildMatch::new(account_id) | ||
| .matches(&outcome_with_receipt.receipt.predecessor_id) | ||
| .matches(outcome_with_receipt.receipt.predecessor_id.as_str()) |
There was a problem hiding this comment.
Latest near-lake-framework changed these to AccountId from String
morgsmccauley
force-pushed
the
feat/count-lake-requests
branch
from
7b59de0 to
bd3d75c
Compare
| @@ -116,6 +116,7 @@ impl BlockStream { | |||
| } | |||
| } | |||
|
|
|||
| #[allow(clippy::too_many_arguments)] | |||
There was a problem hiding this comment.
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.
Yeah - this suppresses the error which I was getting sick of. BlockStreamContext is a good idea.
|
|
||
| // FIX: near lake framework now infinitely retires - we need a way to stop it to allow the test |
There was a problem hiding this comment.
What change to Lake caused infinite retries?
| let response = self | ||
| .s3_client | ||
| .list_objects_v2() | ||
| .max_keys(1000) |
There was a problem hiding this comment.
Is this a value we set somewhere previously or was 1000 the default by the API?
There was a problem hiding this comment.
This is the default from the API, essentially just a copy paste from near_lake_framework code.
| Ok(bytes) | ||
| } | ||
|
|
||
| async fn list_common_prefixes( |
There was a problem hiding this comment.
What is this function used for? Just testing or something else?
There was a problem hiding this comment.
This is part of the S3Client trait exposed by near_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.
Depends on near/near-lake-framework-rs#102
This PR exposes a new metrics which counts the number of Get requests made to S3 by
near-lake-framework. I wanted to start tracking this metric before I merge the change which reduces them, so I can measure the impact of that change. The easiest way to track these requests was to pass a customS3Clienttonear-lake-framework, so we can hook in to the actual requests made.The custom
S3Client(LakeS3Client) is exactly the same as the default implementation innear-lake-frameworkitself, but with the added metric. This is essentially part 1 for #419, as the "reduction" in requests will build on this custom client, adding caching/de-duplication.