-
Notifications
You must be signed in to change notification settings - Fork 224
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
Turn rpc::Client::new
into a sync fn by not performing the health check on creation
#169
Conversation
…` method In addition, both methods now take ownership of the `rpc::Address` rather than cloning its reference. This avoids unecessary clones when the caller already has ownership of the address.
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.
Integration tests contain a few places where the old method signature is used. Other than that the change makes sense. Maybe the new_healthy
method isn't even necessary as callers could simply call health()
themselves (e.g. as a form of heartbeat check).
Yeah, I do remember this feeling weird when I +1 for removing the healthcheck entirely (which already happened, so seems good) |
Codecov Report
@@ Coverage Diff @@
## master #169 +/- ##
==========================================
+ Coverage 39.78% 39.82% +0.04%
==========================================
Files 91 91
Lines 3283 3284 +1
Branches 491 475 -16
==========================================
+ Hits 1306 1308 +2
- Misses 1691 1692 +1
+ Partials 286 284 -2
Continue to review full report at Codecov.
|
My bad, sorry. Alright, I removed the Unfortunately, some integration tests fail for me when running them with
Click here for the full log
|
Yeah, all these |
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.
LGTM 👍 We should prob. update the title of that PR.
rpc::Client::new
to new_healthy
and add sync new
methodrpc::Client::new
into a sync fn by not performing the health check on creation
Just updated the PR title. |
If you want to be sure you didn't introduce a new problem, you can re-check the integration tests against 0.32.7 (together with the changes from #95 <- we should merge that one too). |
* Boilerplate: Add lite-node crate - ran `abscissa new lite-node` - added deps (Cargo.toml) and minimal changes to README.md - add to root workspace * Added config options & copied code into new app crate - copied from tendermint-lite/src/main.rs to lite-node/src/command/start.rs * Delete tendermint-lite: replaced by lite-node * lite -> light * minor improvements to comments / docs minor improvements to comments / docs * Fix a few merge hicks (catch up with latest changes from master) rename some vars and more logical bisection in cmd * fix rebasing hicks * Bucky/abscissify adr (#148) * adr-002 lite client (#54) * adr-002 lite client * finish adr * address Dev's comments * lite -> light * Apply suggestions from code review Co-Authored-By: Ismail Khoffi <Ismail.Khoffi@gmail.com> * updates from review * Apply suggestions from code review Co-Authored-By: Ismail Khoffi <Ismail.Khoffi@gmail.com> * update for better abstraction and latest changes * note about detection * update image. manager -> syncer * update image * update image * More detailed diagram of lite client data flow (#106) * refactor * refactor into more adrs * minor fixes from review * sync adr-003 with latest and call it * rust code blocks Co-authored-by: Ismail Khoffi <Ismail.Khoffi@gmail.com> Co-authored-by: jibrown <jackie.ilana.brown@gmail.com> * working on adr-004 Co-authored-by: Ismail Khoffi <Ismail.Khoffi@gmail.com> Co-authored-by: jibrown <jackie.ilana.brown@gmail.com> * Dealing with the merging in master aftermath * merged in master * Fix merge master fallout (related to #169) * Use `abscissa_tokio` and resolve merge conflicts * New stable rust -> new clippy errs -> fixed Co-authored-by: Ethan Buchman <ethan@coinculture.info> Co-authored-by: jibrown <jackie.ilana.brown@gmail.com>
See #168 for the rationale behind this change.
In addition, both methods now take ownership of the
rpc::Address
rather than cloning a reference. This avoids unnecessary clones when the caller already has ownership of the address.