change default bench-tps client to tpu-client#35335
change default bench-tps client to tpu-client#35335gregcusack merged 4 commits intosolana-labs:masterfrom
bench-tps client to tpu-client#35335Conversation
| Arg::with_name("tpu_client") | ||
| .long("use-tpu-client") | ||
| Arg::with_name("thin_client") | ||
| .long("use-thin-client") | ||
| .conflicts_with("rpc_client") | ||
| .takes_value(false) | ||
| .help("Submit transactions with a TpuClient") |
There was a problem hiding this comment.
We should preserve the --use-tpu-client flag (and the various conflicts_with logic), even though it duplicates the (new) default behavior, to avoid breaking downstream users.
| Arg::with_name("thin_client") | ||
| .long("use-thin-client") | ||
| .conflicts_with("rpc_client") | ||
| .takes_value(false) | ||
| .help("Submit transactions with a TpuClient") | ||
| .help("Submit transactions with a ThinClient") |
There was a problem hiding this comment.
Actually, if we're going to remove this imminently, I think we should avoid exposing a flag for it in the first place.
If there is an internal need to keep it for a bit (eg /net scripts), let's mark this as .hidden(hidden_unless_forced()) so it's not visible. If there's no need for it, let's just remove this Arg.
There was a problem hiding this comment.
Ya if we remove it here we'd have to remove all references to ThinClient in /net scripts in this PR as well. So maybe as you said, let's use: .hidden(hidden_unless_forced()) for now. And then for the PR where we will deprecate ThinClient, we can fully remove this flag and remove all references to ThinClient in /net. does that work?
There was a problem hiding this comment.
Yes, that works for me.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #35335 +/- ##
=======================================
Coverage 81.7% 81.7%
=======================================
Files 834 834
Lines 224235 224235
=======================================
+ Hits 183390 183391 +1
+ Misses 40845 40844 -1 |
CriesofCarrots
left a comment
CriesofCarrots
left a comment
There was a problem hiding this comment.
r+ one last little nit
| .conflicts_with("tpu_client") | ||
| .takes_value(false) | ||
| .hidden(hidden_unless_forced()) | ||
| .help("Submit transactions with a ThinClient") |
There was a problem hiding this comment.
Maybe add a note here that usage of ThinClient is discouraged and that ThinClient will be deprecated?
apfitzge
left a comment
apfitzge
left a comment
There was a problem hiding this comment.
lgtm - gave it a quick test to make sure perf (for my single node use case) was unaffected.
* change default bench-tps client to tpu-client * remote client default to tpu-client * add --use-tpu-client back in. hide --use-thin-client * address nit, inform of future thinclient deprecation
* change default bench-tps client to tpu-client * remote client default to tpu-client * add --use-tpu-client back in. hide --use-thin-client * address nit, inform of future thinclient deprecation
4 participants
Problem
It is time to deprecate and then remove
ThinClientfrombench-tps. This is the first PR to simply just change the defaultbench-tpsclient fromThinClienttoTpuClientSummary of Changes
Change
ThinClienttoTpuClient, add flag for runningthin-clientexplicitly (will be removed in future), and updatenet.shscripts to reflect the change.Fixes #