Skip to content
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.

Sign up for GitHub

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

change default bench-tps client to tpu-client #35335

Merged
merged 4 commits into from
Feb 28, 2024
Merged

change default bench-tps client to tpu-client #35335

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
83b183d
change default bench-tps client to tpu-client
gregcusack Feb 27, 2024
027a589
remote client default to tpu-client
gregcusack Feb 27, 2024
a2ba892
add --use-tpu-client back in. hide --use-thin-client
gregcusack Feb 28, 2024
5d641ce
address nit, inform of future thinclient deprecation
gregcusack Feb 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 21 additions & 18 deletions bench-tps/src/cli.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ pub enum ExternalClientType {

impl Default for ExternalClientType {
fn default() -> Self {
Self::ThinClient
Self::TpuClient
}
}

Expand Down Expand Up @@ -167,19 +167,19 @@ pub fn build_args<'a>(version: &'_ str) -> App<'a, '_> {
.long("rpc-addr")
.value_name("HOST:PORT")
.takes_value(true)
.conflicts_with("tpu_client")
.conflicts_with("rpc_client")
.requires("tpu_addr")
.requires("thin_client")
.help("Specify custom rpc_addr to create thin_client"),
)
.arg(
Arg::with_name("tpu_addr")
.long("tpu-addr")
.value_name("HOST:PORT")
.conflicts_with("tpu_client")
.conflicts_with("rpc_client")
.takes_value(true)
.requires("rpc_addr")
.requires("thin_client")
.help("Specify custom tpu_addr to create thin_client"),
)
.arg(
Expand Down Expand Up @@ -316,30 +316,30 @@ pub fn build_args<'a>(version: &'_ str) -> App<'a, '_> {
.arg(
Arg::with_name("rpc_client")
.long("use-rpc-client")
.conflicts_with("tpu_client")
.conflicts_with("thin_client")
.takes_value(false)
.help("Submit transactions with a RpcClient")
)
.arg(
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")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

.help("Submit transactions with a ThinClient")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that works for me.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a note here that usage of ThinClient is discouraged and that ThinClient will be deprecated?

)
.arg(
Arg::with_name("tpu_disable_quic")
.long("tpu-disable-quic")
.takes_value(false)
.help("Do not submit transactions via QUIC; only affects ThinClient (default) \
or TpuClient sends"),
.help("Do not submit transactions via QUIC; only affects ThinClient \
or TpuClient (default) sends"),
)
.arg(
Arg::with_name("tpu_connection_pool_size")
.long("tpu-connection-pool-size")
.takes_value(true)
.help("Controls the connection pool size per remote address; only affects ThinClient (default) \
or TpuClient sends"),
.help("Controls the connection pool size per remote address; only affects ThinClient \
or TpuClient (default) sends"),
)
.arg(
Arg::with_name("compute_unit_price")
Expand Down Expand Up @@ -442,11 +442,11 @@ pub fn parse_args(matches: &ArgMatches) -> Result<Config, &'static str> {
return Err("could not parse identity path");
}

if matches.is_present("tpu_client") {
args.external_client_type = ExternalClientType::TpuClient;
} else if matches.is_present("rpc_client") {
if matches.is_present("rpc_client") {
args.external_client_type = ExternalClientType::RpcClient;
}
} else if matches.is_present("thin_client") {
args.external_client_type = ExternalClientType::ThinClient;
};

if matches.is_present("tpu_disable_quic") {
args.use_quic = false;
Expand Down Expand Up @@ -679,15 +679,17 @@ mod tests {
}
);

// select different client type
// select different client type and CommitmentConfig
let keypair = read_keypair_file(&keypair_file_name).unwrap();
let matches = build_args("1.0.0").get_matches_from(vec![
"solana-bench-tps",
"--identity",
&keypair_file_name,
"-u",
"http://123.4.5.6:8899",
"--use-tpu-client",
"--use-rpc-client",
"--commitment-config",
"finalized",
]);
let actual = parse_args(&matches).unwrap();
assert_eq!(
Expand All @@ -696,7 +698,8 @@ mod tests {
json_rpc_url: "http://123.4.5.6:8899".to_string(),
websocket_url: "ws://123.4.5.6:8900/".to_string(),
id: keypair,
external_client_type: ExternalClientType::TpuClient,
external_client_type: ExternalClientType::RpcClient,
commitment_config: CommitmentConfig::finalized(),
..Config::default()
}
);
Expand Down
4 changes: 2 additions & 2 deletions net/net.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ Operate a configured testnet
- Enable UDP for tpu transactions

--client-type
- Specify backend client type for bench-tps. Valid options are (thin-client|rpc-client|tpu-client), thin-client is default
- Specify backend client type for bench-tps. Valid options are (thin-client|rpc-client|tpu-client), tpu-client is default

sanity/start-specific options:
-F - Discard validator nodes that didn't bootup successfully
Expand Down Expand Up @@ -834,7 +834,7 @@ waitForNodeInit=true
extraPrimordialStakes=0
disableQuic=false
enableUdp=false
clientType=thin-client
clientType=tpu-client
maybeUseUnstakedConnection=""

command=$1
Expand Down
15 changes: 7 additions & 8 deletions net/remote/remote-client.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,19 +43,19 @@ skip)
exit 1
esac

TPU_CLIENT=false
THIN_CLIENT=false
RPC_CLIENT=false
case "$clientType" in
thin-client)
TPU_CLIENT=false
THIN_CLIENT=true
RPC_CLIENT=false
;;
tpu-client)
TPU_CLIENT=true
THIN_CLIENT=false
RPC_CLIENT=false
;;
rpc-client)
TPU_CLIENT=false
THIN_CLIENT=false
RPC_CLIENT=true
;;
*)
Expand All @@ -74,12 +74,11 @@ solana-bench-tps)

args=()

if ${TPU_CLIENT}; then
args+=(--use-tpu-client)
if ${THIN_CLIENT}; then
args+=(--entrypoint "$entrypointIp:8001")
args+=(--use-thin-client)
elif ${RPC_CLIENT}; then
args+=(--use-rpc-client)
else
args+=(--entrypoint "$entrypointIp:8001")
fi

if [[ -z "$maybeUseUnstakedConnection" ]]; then
Expand Down
Loading