-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
[Indexer-Grpc-V2] Add ConnectionManager. #15750
base: main
Are you sure you want to change the base?
Conversation
⏱️ 33m total CI duration on this PR
🚨 1 job on the last run was significantly faster/slower than expected
|
.collect() | ||
} | ||
|
||
fn maybe_add_sample(&mut self, version: u64, size_bytes: u64) { |
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.
does ringbuffer
make this simpler since the sample size is fixed?
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.
not really, since I'm keeping two VecDeque. (the push
doesn't return the oldest element when it's full in ringbuffer crate)
size_bytes: u64, | ||
} | ||
|
||
pub(crate) struct ConnectionManager { |
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.
nit: some documentation comments here. the ConnectionManager
is critical in the stack based on my understanding.
also i see that String
is often used here as the service address. let's either use url/socketaddr or typedef it to avoid it mixed with regular strings.
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.
take a first round of review; looks good! some nits.
taking a closer look...
55719c5
to
76c1d10
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
❌ Forge suite
|
Description
How Has This Been Tested?
Key Areas to Review
Type of Change
Which Components or Systems Does This Change Impact?
Checklist