add in method for building a `TpuClient` for `LocalCluster` tests by gregcusack · Pull Request #258 · anza-xyz/agave

gregcusack · 2024-03-14T22:52:32Z

A PR to setup the removal of ThinClient from LocalCluster

Problem

Building a TpuClient with a quic cache for tests can be cumbersome. Requires a decent amount of duplicate code.

Summary of Changes

Add methods build_tpu_quic_client and build_tpu_quic_client_with_commitment
Update dos/ and bench-tps/ tests to use the new methods

Note: build_tpu_quic_client_with_commitment is not currently used but will be in future changes

This is the 8th PR on the way to remove ThinClient completely.
See

codecov-commenter · 2024-03-15T00:37:31Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.9%. Comparing base (b3fd87f) to head (af87503).
Report is 18 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #258   +/-   ##
=======================================
  Coverage    81.9%    81.9%           
=======================================
  Files         837      837           
  Lines      226792   226792           
=======================================
+ Hits       185795   185813   +18     
+ Misses      40997    40979   -18

CriesofCarrots

👍 deduping. Commented a few ideas about organization.

CriesofCarrots · 2024-03-15T00:51:06Z

-        connection_cache::ConnectionCache,
-        tpu_client::{TpuClient, TpuClientConfig},
-    },
+    solana_client::tpu_client::{TpuClient, TpuClientConfig},


Let's import solana_tpu_client directly, since the dependency on ConnectionCache is going away. We may be able to remove solana_client entirely, eventually.

ohhh this is good to know. i was actually going to ask about this, since solana_client seems to be just a wrapper around solana_tpu_client. sounds good!

I am wondering if this needs to be separate PRs. Add in with dependency on solana_client and then another PR that switches over the dependencies to solana_tpu_client. A lot of the code including bench-tps relies on solana_client. For example, bench-tps implements BenchTpsClient for solana_client/tpu_client not solana_tpu_client/tpu_client. so switching this up in this PR as well may be large.

If you want to do dependency updates in a separate PR, that's fine with me. I can definitely see the argument for that. Just let me know whether you think it makes more sense to do it before or after this PR.

I think it makes more sense to do it after this PR. This code will be following same usage as previous code (aka using solana_client::tpu_client. I have another PR in drafts that will then switch over to using solana-tpu-client. Then I will make another PR that finally switches over ThinClient in LocalCluster to solana-tpu-client.

CriesofCarrots · 2024-03-15T00:54:19Z

-    solana_client::{connection_cache::ConnectionCache, thin_client::ThinClient},
+    solana_client::{
+        connection_cache::ConnectionCache,
+        rpc_client::RpcClient,


Let's make this dependency on solana_rpc_client directly, so that it's more obvious when it's time to remove solana_client

CriesofCarrots · 2024-03-15T00:54:41Z

+        connection_cache::ConnectionCache,
+        rpc_client::RpcClient,
+        thin_client::ThinClient,
+        tpu_client::{TpuClient, TpuClientConfig},


Ditto about solana_tpu_client

CriesofCarrots · 2024-03-15T01:01:41Z

+pub fn build_tpu_quic_client(
+    cluster: &LocalCluster,
+) -> Result<TpuClient<QuicPool, QuicConnectionManager, QuicConfig>> {
+    build_tpu_client(cluster, |rpc_url| Arc::new(RpcClient::new(rpc_url)))
+}
+
+pub fn build_tpu_quic_client_with_commitment(
+    cluster: &LocalCluster,
+    commitment_config: CommitmentConfig,
+) -> Result<TpuClient<QuicPool, QuicConnectionManager, QuicConfig>> {
+    build_tpu_client(cluster, |rpc_url| {
+        Arc::new(RpcClient::new_with_commitment(rpc_url, commitment_config))
+    })
+}


What do you think about extending trait Cluster with these methods? (And maybe down the road deprecating Cluster::get_validator_client(), which returns a ThinClient...)

haha in the process! working on another PR for extending trait Cluster

Okay, great. Let's not bother adding these public, stand-alone methods, then.

CriesofCarrots · 2024-03-15T01:01:55Z

+fn build_tpu_client<F>(
+    cluster: &LocalCluster,


This seems like it should be a method on LocalCluster.

ya that's a good point. will update

apfitzge · 2024-03-15T01:26:54Z


+pub fn build_tpu_quic_client(
+    cluster: &LocalCluster,
+) -> Result<TpuClient<QuicPool, QuicConnectionManager, QuicConfig>> {


minor: is there no alias for this Quic version of the TpuClient?
if there is, let's re-use that, if not no need to add.

as it turns out there is here: https://github.com/gregcusack/solana/blob/31d04a8d6f0550bf4dac7dab6cef34fb5e00483a/client/src/send_and_confirm_transactions_in_parallel.rs#L37
Could possibly make this more accessible and then use it everywhere its needed?

yeah feel free to do that separately, but can probably just pub it or define in appropriate place.

the problem is that the QuicTpuClient defined in that link above is using a different TpuClient than the one I need. It is using this instead of what we need, which is this TpuClient. Although this will change in the next PR. Since slowly going away from solana_client/tpu_client in favor of tpu_client/

…za-xyz#258) * add in method for building a TpuClient for LocalCluster tests * add cluster trait. leave dependency on solana_client::tpu_client

gregcusack force-pushed the tpu-client-from-entrypoint branch 2 times, most recently from caa9571 to 664a003 Compare March 14, 2024 23:04

add in method for building a TpuClient for LocalCluster tests

54ca966

gregcusack force-pushed the tpu-client-from-entrypoint branch from 664a003 to 54ca966 Compare March 14, 2024 23:10

gregcusack marked this pull request as ready for review March 15, 2024 00:37

gregcusack requested review from CriesofCarrots, KirillLykov and apfitzge March 15, 2024 00:37

CriesofCarrots reviewed Mar 15, 2024

View reviewed changes

apfitzge reviewed Mar 15, 2024

View reviewed changes

gregcusack force-pushed the tpu-client-from-entrypoint branch from c1e4b6c to 6a30412 Compare March 16, 2024 16:04

gregcusack mentioned this pull request Mar 18, 2024

switch to solana-tpu-client from solana_client::tpu_client for bench-tps, dos/, LocalCluster, gossip/ #301

Closed

gregcusack requested a review from CriesofCarrots March 18, 2024 21:06

add cluster trait. leave dependency on solana_client::tpu_client

af87503

gregcusack force-pushed the tpu-client-from-entrypoint branch from 6a30412 to af87503 Compare March 18, 2024 21:13

CriesofCarrots approved these changes Mar 18, 2024

View reviewed changes

gregcusack merged commit ed573ff into anza-xyz:master Mar 19, 2024

gregcusack deleted the tpu-client-from-entrypoint branch March 19, 2024 00:58

gregcusack mentioned this pull request Mar 19, 2024

switch to solana-tpu-client from solana_client::tpu_client for bench-tps, dos/, LocalCluster, gossip/ #310

Merged

This was referenced May 8, 2024

Remove ThinClient from LocalCluster #977

Closed

Remove ThinClient from LocalCluster #1300

Merged

Conversation

gregcusack commented Mar 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Summary of Changes

Uh oh!

codecov-commenter commented Mar 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

CriesofCarrots left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gregcusack commented Mar 14, 2024 •

edited

Loading

codecov-commenter commented Mar 15, 2024 •

edited

Loading