Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Health endpoint #9847

Merged
merged 7 commits into from
Nov 7, 2018
Merged

Health endpoint #9847

merged 7 commits into from
Nov 7, 2018

Conversation

tomusdrw
Copy link
Collaborator

@tomusdrw tomusdrw commented Nov 1, 2018

Closes #7560
Closes #9401

Introduces new RPC method parity_nodeStatus that return OK in case the node is not syncing and you have at least one peer (exception: development chain).

This method is then used as a health API of HTTP server.

@tomusdrw tomusdrw added A0-pleasereview 🤓 Pull request needs code review. M6-rpcapi 📣 RPC API. labels Nov 1, 2018
@5chdn 5chdn added this to the 2.3 milestone Nov 1, 2018
Copy link
Collaborator

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

Looks good, but I found the if in status_error a bit hard to spot i.e, to determine whether the node has no peers or is syncing!

@niklasad1 niklasad1 added the A8-looksgood 🦄 Pull request is reviewed well. label Nov 1, 2018
@niklasad1
Copy link
Collaborator

@tomusdrw shall this be backported?

@tomusdrw
Copy link
Collaborator Author

tomusdrw commented Nov 2, 2018

@niklasad1 no, I don't think backporting is required - it's a new feature not really a bugfix.

@@ -481,4 +487,21 @@ impl<C, M, U, S> Parity for ParityClient<C, M, U> where
fn submit_work_detail(&self, nonce: H64, pow_hash: H256, mix_hash: H256) -> Result<H256> {
helpers::submit_work_detail(&self.client, &self.miner, nonce, pow_hash, mix_hash)
}

fn status(&self) -> Result<()> {
let has_peers = self.client.spec_name() == "developmentchain" || self.sync.status().num_peers > 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we have a more generic method to check if it's the dev chain? Or couldn't it be the same check as for the light implementation?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point, was trying to look for any detection we do already, but haven't found anything.
In light we don't really have spec_name exposed, but I agree that current way is suboptimal.
I'll try to figure something out.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ngotchac addressed, could you have a look?

@tomusdrw tomusdrw added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. A0-pleasereview 🤓 Pull request needs code review. and removed A0-pleasereview 🤓 Pull request needs code review. A8-looksgood 🦄 Pull request is reviewed well. A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Nov 5, 2018
@Tbaut
Copy link
Contributor

Tbaut commented Nov 5, 2018

Also please don't forget the doc 👼

@ngotchac ngotchac added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Nov 5, 2018
@niklasad1 niklasad1 added A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. and removed A8-looksgood 🦄 Pull request is reviewed well. labels Nov 5, 2018
@tomusdrw tomusdrw added A8-looksgood 🦄 Pull request is reviewed well. and removed A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. labels Nov 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M6-rpcapi 📣 RPC API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants