-
Notifications
You must be signed in to change notification settings - Fork 254
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
Remove ThinClient
from dos/
#117
Remove ThinClient
from dos/
#117
Conversation
21e39ec
to
70324f5
Compare
3edaa52
to
f2e0867
Compare
let (rpc_addrs, tpu_addrs): (Vec<_>, Vec<_>) = nodes | ||
.iter() | ||
.filter_map(|node| node.valid_client_facing_addr(protocol, socket_addr_space)) | ||
.unzip(); | ||
let num_nodes = tpu_addrs.len(); | ||
( | ||
ThinClient::new_from_addrs(rpc_addrs, tpu_addrs, connection_cache), | ||
num_nodes, | ||
) |
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 seems to be spinning up rpc multiple clients under the wrapper of a ThinClient
. I was able to get similar performance with a single TPU Quic client. This similar performance is why I changed it to a single TPU Client. However, I am not sure I have full context as to whether solana-dos
needs to spin up multiple TPU clients for reasons other than maximizing rate_per_second
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #117 +/- ##
========================================
Coverage 81.8% 81.8%
========================================
Files 838 838
Lines 226389 226273 -116
========================================
- Hits 185342 185263 -79
+ Misses 41047 41010 -37 |
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.
looks good, the only thing is that this PR does two things:
- ThinClient -> TpuClient in dos
- removes ThinClient from bench-tps
I would split these changes because they are unrelated from what I see.
dos/src/main.rs
Outdated
let rpc_pubsub_url = format!("ws://{}/", cluster.entry_point_info.rpc_pubsub().unwrap()); | ||
let rpc_url = format!("http://{}", cluster.entry_point_info.rpc().unwrap()); | ||
|
||
let ConnectionCache::Quic(cache) = &*cluster.connection_cache 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.
&*
looks a bit suspicious
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.
Ya tbh i thought so too lol. But it is used in ThinClient
here, here, and here.
Also looks like it is a valid way to get a reference to the internal structure in an Arc. See: https://doc.rust-lang.org/beta/std/sync/struct.Condvar.html and https://stackoverflow.com/questions/62651479/understanding-to-access-a-rust-arc
dos/src/main.rs
Outdated
.unwrap(), | ||
cluster.connection_cache.clone(), | ||
)); | ||
let rpc_pubsub_url = format!("ws://{}/", cluster.entry_point_info.rpc_pubsub().unwrap()); |
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.
myabe move this common code to create TpuClient to some helper function in the scope of tests submodule?
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. will do
78622fb
to
cf58be6
Compare
7d09b50
cf58be6
to
7d09b50
Compare
* remove `ThinClient` from `dos/` and replace `ThinClient` with `TpuClient` * remove test for valid_client_facing_addr since it is no longer used
5th PR on the way to remove ThinClient completely.
See
bench-tps
client totpu-client
solana-labs/solana#35335ThinClient
and removeThinClient
frombench-tps
solana-labs/solana#35365get_client
andget_multi_client
#177get_client
andget_multi_client
#184Problem
ThinClient
is deprecated, so we need to removesolana-dos
's reliance on itSummary of Changes
Use
TpuClient
insolana-dos
instead ofThinClient