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

Jump to bottom

rpc: add Client::wait_until_healthy method #855

Merged
merged 1 commit into from
Apr 6, 2021
Merged

rpc: add Client::wait_until_healthy method #855

merged 1 commit into from
Apr 6, 2021

Conversation

tony-iqlusion
Copy link
Collaborator

Adds a method which repeatedly polls the /health endpoint until it returns a successful response or a specified timeout elapses.

This is useful when writing tests that need to wait for a node to boot up (e.g. inside of a docker container), e.g.:

https://github.com/cosmos/cosmos-rust/pull/71/files#diff-0e31f23fc223d1d543169a682e0f1093c1cc53c6a312febce7d5d1854919fafaR84-R96

@tarcieri tarcieri requested a review from thanethomson April 6, 2021 20:13
@tarcieri tarcieri force-pushed the tony-iqlusion/rpc-wait-until-healthy branch 2 times, most recently from d107113 to c86f077 Compare April 6, 2021 20:50
thanethomson
thanethomson reviewed Apr 6, 2021
View reviewed changes
Copy link
Contributor

@thanethomson thanethomson left a comment

Choose a reason for hiding this comment

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

This is a great idea! Have you tested it locally on your side?

rpc/src/client.rs Outdated Show resolved Hide resolved
@tony-iqlusion
rpc: add Client::wait_until_healthy method
2898474 
Adds a method which repeatedly polls the `/health` endpoint until it
returns a successful response or a specified timeout elapses.

This is useful when writing tests that need to wait for a node to boot
up (e.g. inside of a docker container).
@tarcieri tarcieri force-pushed the tony-iqlusion/rpc-wait-until-healthy branch from c86f077 to 2898474 Compare April 6, 2021 21:30
@tony-iqlusion
Copy link
Collaborator Author

I updated the PR linked in the description to use code that is now nearly identical to this and it's working as expected locally:

https://github.com/cosmos/cosmos-rust/pull/71/files#diff-0e31f23fc223d1d543169a682e0f1093c1cc53c6a312febce7d5d1854919fafaR97-R117

@thanethomson thanethomson changed the base branch from master to release/v0.19.0 April 6, 2021 22:11
thanethomson
thanethomson reviewed Apr 6, 2021
View reviewed changes
Copy link
Contributor

@thanethomson thanethomson left a comment

Choose a reason for hiding this comment

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

Awesome. I'd like to include this in the v0.19.0 release. Could you please add this to the CHANGELOG under the IMPROVEMENTS section?

@thanethomson
Copy link
Contributor

Actually don't worry about it - I'll add something tomorrow when I finalize the release. It'll be easier that way so we don't have merge conflicts in the changelog.

@thanethomson thanethomson merged commit 2cdab92 into release/v0.19.0 Apr 6, 2021
@thanethomson thanethomson deleted the tony-iqlusion/rpc-wait-until-healthy branch April 6, 2021 23:57
thanethomson added a commit that referenced this pull request Apr 7, 2021
@thanethomson
Add CHANGELOG entry for #855
6040a76 
Signed-off-by: Thane Thomson <[email protected]>
thanethomson added a commit that referenced this pull request Apr 7, 2021
@thanethomson @tony-iqlusion
Release v0.19.0 (#854)
2609b60 
* Update changelog for v0.19.0 release

Signed-off-by: Thane Thomson <[email protected]>

* Bump versions to v0.19.0

Signed-off-by: Thane Thomson <[email protected]>

* Add minor documentation for tendermint-light-client-js crate

Signed-off-by: Thane Thomson <[email protected]>

* Add abci and light-client-js crates to release script

Signed-off-by: Thane Thomson <[email protected]>

* Add cargo metadata for abci and light-client-js

Signed-off-by: Thane Thomson <[email protected]>

* Fix keywords in abci and light-client-js crates

Signed-off-by: Thane Thomson <[email protected]>

* rpc: add `Client::wait_until_healthy` method (#855)

Adds a method which repeatedly polls the `/health` endpoint until it
returns a successful response or a specified timeout elapses.

This is useful when writing tests that need to wait for a node to boot
up (e.g. inside of a docker container).

* Temporarily remove RPC query parsing (#858)

Signed-off-by: Thane Thomson <[email protected]>

* Add CHANGELOG entry for #855

Signed-off-by: Thane Thomson <[email protected]>

Co-authored-by: Tony Arcieri (iqlusion) <[email protected]>
@tony-iqlusion
Copy link
Collaborator Author

tony-iqlusion commented Apr 7, 2021 •
edited
Loading

I noticed one small problem with this trying to use it.

After updating to tendermint-rpc v0.19, I was still using tcp:// URLs, and even though those were failing to parse, this code waited the full timeout and just said it timed out, making it a bit hard to debug.

Perhaps it should look for certain kinds of errors, i.e. ones relating to the remote node being down, and bubble all others up. Alternatively e.g. HttpClient::new could return a Result and error out eagerly if the URL fails to parse.

Otherwise I switched my code to use it and it's working great!

@thanethomson
Copy link
Contributor

After updating to tendermint-rpc v0.19, I was still using tcp:// URLs, and even though those were failing to parse, this code waited the full timeout and just said it timed out, making it a bit hard to debug.

Yes, I think I've just found a small bug in the rpc::Url -> uri::Uri conversion that would cause this to happen when specifying a tcp:// address. I'll open an issue 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thanethomson thanethomson thanethomson left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tony-iqlusion @thanethomson