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

Remove node-health#9119

Merged
debris merged 6 commits into
masterfrom
am-nodehealth
Jul 18, 2018
Merged

Remove node-health#9119
debris merged 6 commits into
masterfrom
am-nodehealth

Conversation

@amaury1093
Copy link
Copy Markdown
Contributor

@amaury1093 amaury1093 commented Jul 13, 2018

  • Remove node-health and the parity_nodeHealth RPC
  • Remove ntp_servers, as it was only used by node-health to check if local clock is sync.

Parity UI starting from v0.3.0 is free from parity_nodeHealth calls. The info returned by node-health has been replaced by:

  • eth_syncing for sync state
  • parity_netPeers for network state
  • a JS implementation of whether the clock is sync or not.

@amaury1093 amaury1093 added the A0-pleasereview 🤓 Pull request needs code review. label Jul 13, 2018
@5chdn 5chdn added the M4-core ⛓ Core client code / Rust. label Jul 14, 2018
@5chdn 5chdn added this to the 2.1 milestone Jul 14, 2018
@5chdn 5chdn mentioned this pull request Jul 14, 2018
Merged
15 tasks
ascjones
ascjones reviewed Jul 16, 2018
View reviewed changes
Comment thread parity/cli/mod.rs
ARG arg_ntp_servers: (String) = "0.parity.pool.ntp.org:123,1.parity.pool.ntp.org:123,2.parity.pool.ntp.org:123,3.parity.pool.ntp.org:123", or |c: &Config| c.misc.as_ref()?.ntp_servers.clone().map(|vec| vec.join(",")),
"--ntp-servers=[HOSTS]",
"Comma separated list of NTP servers to provide current time (host:port). Used to verify node health. Parity uses pool.ntp.org NTP servers; consider joining the pool: http://www.pool.ntp.org/join.html",

Copy link
Copy Markdown
Contributor

@ascjones ascjones Jul 16, 2018

Choose a reason for hiding this comment

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

I believe this should be deprecated as per: #9036

Copy link
Copy Markdown
Contributor Author

@amaury1093 amaury1093 Jul 16, 2018

Choose a reason for hiding this comment

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

Done.

Copy link
Copy Markdown
Contributor

@ascjones ascjones Jul 16, 2018

Choose a reason for hiding this comment

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

It also needs to be added to https://github.com/paritytech/parity/blob/master/parity/deprecated.rs, so that it produces a warning message if the deprecated arg is used.

Copy link
Copy Markdown
Contributor Author

@amaury1093 amaury1093 Jul 16, 2018

Choose a reason for hiding this comment

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

Right, thanks!

@5chdn 5chdn requested a review from ltfschoen July 16, 2018 10:13
ascjones
ascjones approved these changes Jul 16, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@ascjones ascjones left a comment

Choose a reason for hiding this comment

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

LGTM

@ascjones ascjones added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jul 16, 2018
@5chdn 5chdn mentioned this pull request Jul 17, 2018
Merged
15 tasks
andresilva
andresilva reviewed Jul 17, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@andresilva andresilva left a comment

Choose a reason for hiding this comment

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

Minor nits.

Comment thread rpc/src/v1/impls/parity.rs Outdated
use updater::{Service as UpdateService};
use jsonrpc_core::{BoxFuture, Result};
use jsonrpc_core::futures::{future, Future};
use jsonrpc_core::futures::{future};
Copy link
Copy Markdown
Contributor

@andresilva andresilva Jul 17, 2018

Choose a reason for hiding this comment

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

use jsonrpc_core::futures::future;

Comment thread parity/cli/mod.rs Outdated
color: Option<bool>,
ports_shift: Option<u16>,
unsafe_expose: Option<bool>,
_legacy_ntp_servers: Option<Vec<String>>,
Copy link
Copy Markdown
Contributor

@andresilva andresilva Jul 17, 2018

Choose a reason for hiding this comment

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

I know we did this for some other deprecated options, but do we need to keep this fields? Seems like it isn't used anywhere besides when parsing the argument. We can use or |_| None in that case.

Copy link
Copy Markdown
Contributor Author

@amaury1093 amaury1093 Jul 17, 2018

Choose a reason for hiding this comment

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

So I removed this field in my last commit. But the other ones are still there. Probably to be removed in the scope of another pr?

Comment thread rpc/src/v1/tests/mocked/parity.rs Outdated
use ethstore::ethkey::{Generator, Random};
use miner::pool::local_transactions::Status as LocalTransactionStatus;
use node_health::{self, NodeHealth};
use parity_reactor;
Copy link
Copy Markdown
Contributor

@andresilva andresilva Jul 17, 2018

Choose a reason for hiding this comment

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

Unused import.

@andresilva
Copy link
Copy Markdown
Contributor

andresilva commented Jul 17, 2018

The NTP code was my first contribution to parity 😛.

@5chdn 5chdn modified the milestones: 2.1, 2.2 Jul 17, 2018
debris
debris approved these changes Jul 18, 2018
View reviewed changes
Copy link
Copy Markdown
Collaborator

@debris debris left a comment

Choose a reason for hiding this comment

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

lgtm! But before merge, I would like @tomusdrw to also sign off on this

@debris debris requested a review from tomusdrw July 18, 2018 12:14
tomusdrw
tomusdrw approved these changes Jul 18, 2018
View reviewed changes
Copy link
Copy Markdown
Collaborator

@tomusdrw tomusdrw 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!

@debris debris merged commit 3c27587 into master Jul 18, 2018
@debris debris deleted the am-nodehealth branch July 18, 2018 14:27
5chdn pushed a commit that referenced this pull request Jul 24, 2018
@amaury1093 @5chdn
Remove node-health (#9119)
1d53ce4 
* Remove node-health

* Remove ntp_servers

* Add --ntp-servers as legacy instead of removing it

* Add --ntp-servers to deprecated args

* Remove unused stuff

* Remove _legacy_ntp_servers
5chdn added a commit that referenced this pull request Jul 26, 2018
@5chdn
Backports to 2.0.1-beta (#9145)
e7dc0be 
* parity-version: bump beta to 2.0.1

* ci: update version strings for snaps

* Be more graceful on Aura difficulty validation (#9164)

* Be more graceful on Aura difficulty validation

* test: rejects_step_backwards

* test: proposer_switching

* test: rejects_future_block

* test: reports_skipped

* test: verify_empty_seal_steps

* Remove node-health (#9119)

* Remove node-health

* Remove ntp_servers

* Add --ntp-servers as legacy instead of removing it

* Add --ntp-servers to deprecated args

* Remove unused stuff

* Remove _legacy_ntp_servers

* parity: fix UserDefaults json parser (#9189)

* parity: fix UserDefaults json parser

* parity: use serde_derive for UserDefaults

* parity: support deserialization of old UserDefault json format

* parity: make UserDefaults serde backwards compatible

* parity: tabify indentation in UserDefaults

* Fix bugfix hard fork logic (#9138)

* Fix bugfix hard fork logic

* Remove dustProtectionTransition from bugfix category

EIP-168 is not enabled by default

* Remove unnecessary 'static

* Disable per-sender limit for local transactions. (#9148)

* Disable per-sender limit for local transactions.

* Add a missing new line.

* rpc: fix is_major_importing sync state condition (#9112)

* rpc: fix is_major_importing sync state condition

* rpc: fix informant printout when waiting for peers

* fix verification in ethcore-sync collect_blocks (#9135)

* docker: update hub dockerfile (#9173)

* update Dockerfile for hub

update to Ubuntu Xenial 16.04
fix cmake version

* docker: fix tab indentation in hub dockerfile

* rpc: fix broken merge

* rcp: remove node_health leftover from merge

* rpc: remove dapps leftover from merge
@ddorgan ddorgan mentioned this pull request Oct 17, 2018
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@ltfschoen ltfschoen Awaiting requested review from ltfschoen

4 more reviewers

@andresilva andresilva andresilva left review comments

@ascjones ascjones ascjones approved these changes

@tomusdrw tomusdrw tomusdrw approved these changes

@debris debris debris approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.

Projects

None yet

Milestone

2.1 Whatever

Development

Successfully merging this pull request may close these issues.

6 participants

@amaury1093 @andresilva @ascjones @tomusdrw @debris @5chdn