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.

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

Deprecate ThinClient and remove ThinClient from bench-tps #35365

Merged

Conversation

gregcusack
Copy link
Contributor

@gregcusack gregcusack commented Feb 29, 2024

2nd PR on the way to remove ThinClient completely.
See first PR: #35335

Problem

It is time to deprecate ThinClient as it is rarely used and only sends to one leader.

Summary of Changes

  • Deprecate ThinClient
  • Remove ThinClient from bench-tps

Note: I left in BenchTpsClient implementation for ThinClient since dos/ requires ThinClient as of now. Next PR will remove the dos/ ThinClient dependency.

@gregcusack gregcusack added the work in progress This isn't quite right yet label Feb 29, 2024
@gregcusack gregcusack force-pushed the remove-thin-client-from-bench-tps branch from ed2c800 to 9eb34f2 Compare February 29, 2024 02:18
@apfitzge apfitzge self-requested a review February 29, 2024 19:53
Copy link

codecov bot commented Feb 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.7%. Comparing base (6ee3bb9) to head (ed7dd17).
Report is 30 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff            @@
##           master   #35365    +/-   ##
========================================
  Coverage    81.7%    81.7%            
========================================
  Files         834      834            
  Lines      224299   224844   +545     
========================================
+ Hits       183361   183906   +545     
  Misses      40938    40938            

@CriesofCarrots
Copy link
Contributor

Is this WIP or ready for review?

@gregcusack gregcusack removed the work in progress This isn't quite right yet label Feb 29, 2024
@gregcusack
Copy link
Contributor Author

Is this WIP or ready for review?

oh yes sorry ready for review! forgot to remove the Work in Progress tag.

thin-client/src/thin_client.rs Outdated Show resolved Hide resolved
client/src/thin_client.rs Show resolved Hide resolved
bench-tps/src/cli.rs Outdated Show resolved Hide resolved
@CriesofCarrots
Copy link
Contributor

Oh also: in future, when you're ready for review, it would be helpful if you could clean up the commits so they are concise and distinct. It's nice to review by commit, but easy to get bogged down when there are changes that are temporary, for debug only, etc.

@gregcusack
Copy link
Contributor Author

Oh also: in future, when you're ready for review, it would be helpful if you could clean up the commits so they are concise and distinct. It's nice to review by commit, but easy to get bogged down when there are changes that are temporary, for debug only, etc.

ok will fix this! thank you for the tip

@gregcusack gregcusack force-pushed the remove-thin-client-from-bench-tps branch 3 times, most recently from aab78b7 to 0e93c54 Compare March 1, 2024 05:40
add back in command line args for thinclient. add thin-client deprecation README

refactor TpuClient connection
@gregcusack gregcusack force-pushed the remove-thin-client-from-bench-tps branch from 0e93c54 to 3ce13e3 Compare March 1, 2024 05:44
Copy link
Contributor

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

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

r+ version nits

Looks great, thanks for the polish!
Much better commits, although if you find yourself listing multiple things in one commit (as in your 2nd one), those changes should usually be separated. No need to churn this one any further, though :)

@@ -111,6 +111,7 @@ impl ClientOptimizer {
}

/// An object for querying and sending transactions to the network.
#[deprecated(since = "2.0.0", note = "Use [RpcClient] or [TpuClient] instead.")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#[deprecated(since = "2.0.0", note = "Use [RpcClient] or [TpuClient] instead.")]
#[deprecated(since = "1.19.0", note = "Use [RpcClient] or [TpuClient] instead.")]

Since this is the repo's current version, let's go with this for now. I'll mention it to releng, but hopefully we can all remember to update these to v2 when that version bump lands.

@@ -0,0 +1,4 @@
# thin-client
This crate for `thin-client` is deprecated as of v2.0.0. It will receive no bugfixes or updates.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
This crate for `thin-client` is deprecated as of v2.0.0. It will receive no bugfixes or updates.
This crate for `thin-client` is deprecated as of v1.19.0. It will receive no bugfixes or updates.

For now...

KirillLykov
KirillLykov previously approved these changes Mar 1, 2024
@gregcusack gregcusack merged commit 5f6d66e into solana-labs:master Mar 1, 2024
45 checks passed
@gregcusack gregcusack deleted the remove-thin-client-from-bench-tps branch March 1, 2024 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants