Remove Old Service, 2nd try by coriolinus · Pull Request #1732 · paritytech/polkadot

coriolinus · 2020-09-18T09:23:05Z

#1630 was immediately reverted in #1731 due to insufficient testing. This PR contains the same changeset from #1630 in a form which can be merged after future testing.

i.e. Revert "Revert "Remove service, migrate all to service-new (#1630)" (#1731)" This reverts commit c68aee3. This allows us to get the changeset from #1630 into a new branch which can be merged sometime in the future after appropriate burnin tests have completed.

node/service/Cargo.toml

* master: Bump jsonrpc-core to v15 (#1737) Companion PR for #6215 (#1654) Companion PR for #7138 (WeightInfo for Scheduler) (#1734) Companion PR for Bounties #5715 (#1336)

…ytech/polkadot into prgn-remove-old-service-second-try * 'prgn-remove-old-service-second-try' of github.com:paritytech/polkadot: rename polkadot-service-new -> polkadot-service

ordian

The code changes were already reviewed in #1630.

This PR has been on burnin since the last Friday. With the help of @mxinden I was not able to spot any anomalies. Please approve if you have no objections to merge it.

* master: Update to substrate 2.0 (#1744) Companion: Handle construct_runtime breaking change. (#1692) Companion for `ModuleToIndex` to `PalletInfo` rename (#1743) Companion for substrate/pull/7161 (#1739) Companion for 7155 (WeightInfo for Babe and Grandpa) (#1736) Companion PR for #7136 (WeightInfo for Session / Offences) (#1735)

bkchr · 2020-09-23T21:22:39Z

The code changes were already reviewed in #1630.

This PR has been on burnin since the last Friday. With the help of @mxinden I was not able to spot any anomalies. Please approve if you have no objections to merge it.

Was it now also tested on Polkadot?

bkchr

Besides my comments, I think it is okay.

A general note, the old service file should have been copied to the node/service directory and the 3 changes related to the new architecture should have been applied. Now it was much more complicated to review this, to make sure nothing was forgotten.

runtime/test-runtime/src/lib.rs

runtime/rococo-v1/README.md

node/service/src/lib.rs

bkchr · 2020-09-24T07:55:44Z

node/service/src/lib.rs

+/// This is an advanced feature and not recommended for general use. Generally, `build_full` is
+/// a better choice.
+#[cfg(feature = "full-node")]
+pub fn new_full<RuntimeApi, Executor>(


In the old service this also got a parameter "test" which was forgotten here.

This should be brought back.

Could you elaborate why is it needed. I see it used in two places:

polkadot/service/src/lib.rs

Lines 149 to 154 in 3b37ea6

if !test {

// If we're using prometheus, use a registry with a prefix of `polkadot`.

if let Some(PrometheusConfig { registry, .. }) = config.prometheus_config.as_mut() {

*registry = Registry::new_custom(Some("polkadot".into()), None)?;

}

}

polkadot/service/src/lib.rs

Lines 171 to 175 in 3b37ea6

let grandpa_hard_forks = if config.chain_spec.is_kusama() && !test {

crate::grandpa_support::kusama_hard_forks()

} else {

Vec::new()

};

Is it only to set the prometheus prefix/hardfork kusama or am I missing something?

Maybe we can ignore it, I'm not sure. I don't have implemented this.

@cecton was this AFAIK.

It's because the test service uses the test runtime and don't need prometheus.

But for the second place with kusama hard forks I think the condition !test can safely be removed because the test config.chain_spec.is_kusama() will be false for the test runtime anyway.

Originally there was one important other thing: the ValidationPool is built differently for the test node: https://github.com/paritytech/polkadot/blob/rococo-branch/service/src/lib.rs#L414

It's because the test service uses the test runtime and don't need prometheus.

but if prometheus_config.is_none(), this line will not be executed anyway, so it seems redundant

Originally there was one important other thing: the ValidationPool is built differently for the test node: https://github.com/paritytech/polkadot/blob/rococo-branch/service/src/lib.rs#L414

do you think it's worth adding back to master or it can be taken care of in the rococo-branch?

do you think it's worth adding back to master or it can be taken care of in the rococo-branch?

I'm pretty sure rococo-branch is temporary (@bkchr correct me if I'm wrong). We will need to get it back or add a feature of some sort.

but if prometheus_config.is_none(), this line will not be executed anyway, so it seems redundant

You're right!! I think it's because new_full used to be a macro so the code was compiled on cumulus where we don't have the dependency for that. Now it's not an issue anymore. 🎉

🤔 comes to think of it... it was the same thing for the kusama forks thingy.

Cool!! We can drop this safely

ordian · 2020-09-24T10:17:05Z

Was it now also tested on Polkadot?

Yes, it was tested on a sentry node, also sentry and validator on kusama.

Thanks for the review, I'll apply the changes while Peter is out.

bkchr · 2020-09-24T10:27:15Z

It should also be tested on a Polkadot validator.

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

* master: provisioner tests: remove tokio from dev-dependencies (#1745)

bkchr

After it is tested on a Polkadot validator, it should be fine to merge.

runtime/rococo-v1/README.md

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

coriolinus · 2020-09-28T08:20:56Z

Burnin on a Polkadot validator has been running for four days now, and it seems to be operating normally.

coriolinus · 2020-09-28T08:22:56Z

bot merge

ghost · 2020-09-28T08:23:01Z

Trying merge.

This reverts commit 42fbca6.

* master: Collator protocol followup (#1741) Revert "Remove Old Service, 2nd try (#1732)" (#1758) Remove Old Service, 2nd try (#1732) collation-generation: guide and tidying (#1753) Companion for #7111 (Introduce `cancel_proposal` and `blacklist`) (#1728) Parachains: Introduce a dummy module to include the Origin. (#1749) provisioner tests: remove tokio from dev-dependencies (#1745)

i.e. Revert "Revert "Remove Old Service, 2nd try (#1732)" (#1758)" This reverts commit c80f7b6. Closes #1757. We now have some evidence that the polkadot validator was producing blocks after all; the reason the blocks_constructed metric was 0 was that as a new metric it hadn't yet been incorporated into that branch's codebase. See #1757 (comment) As this PR is based on a newer `master` branch than the previous one, that should hopefully no longer be an issue.

* Remove old service, 3rd try i.e. Revert "Revert "Remove Old Service, 2nd try (#1732)" (#1758)" This reverts commit c80f7b6. Closes #1757. We now have some evidence that the polkadot validator was producing blocks after all; the reason the blocks_constructed metric was 0 was that as a new metric it hadn't yet been incorporated into that branch's codebase. See #1757 (comment) As this PR is based on a newer `master` branch than the previous one, that should hopefully no longer be an issue. * paras trait now has an Origin type * initial work running a two node local net * use the right incantations so the nodes produce blocks together * improve internal documentation Co-authored-by: Bastian Köcher <git@kchr.de>

coriolinus added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. A1-needsburnin labels Sep 18, 2020

coriolinus self-assigned this Sep 18, 2020

paritytech-ci added the D2-breaksapi label Sep 18, 2020

coriolinus added 3 commits September 18, 2020 11:31

remove ',)' from codebase outside of macros

372f4f6

restore bdfl-preferred formatting

b7b45b3

attempt to improve destructuring formatting

944b745

andresilva reviewed Sep 18, 2020

View reviewed changes

node/service/Cargo.toml Show resolved Hide resolved

coriolinus and others added 3 commits September 18, 2020 12:36

rename polkadot-service-new -> polkadot-service

838248b

Merge branch 'master' into prgn-remove-old-service-second-try

036080b

* master: Bump jsonrpc-core to v15 (#1737) Companion PR for #6215 (#1654) Companion PR for #7138 (WeightInfo for Scheduler) (#1734) Companion PR for Bounties #5715 (#1336)

ordian approved these changes Sep 22, 2020

View reviewed changes

ordian requested review from andresilva and bkchr September 22, 2020 09:57

bkchr reviewed Sep 24, 2020

View reviewed changes

ordian and others added 3 commits September 24, 2020 13:01

Apply suggestions from code review

fd87f00

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

Merge branch 'master' into prgn-remove-old-service-second-try

a6ca1d4

* master: provisioner tests: remove tokio from dev-dependencies (#1745)

remove unused import

032ea21

bkchr approved these changes Sep 24, 2020

View reviewed changes

runtime/rococo-v1/README.md Outdated Show resolved Hide resolved

Update runtime/rococo-v1/README.md

c230ae9

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

ghost merged commit 42fbca6 into master Sep 28, 2020

ghost deleted the prgn-remove-old-service-second-try branch September 28, 2020 08:23

coriolinus mentioned this pull request Sep 28, 2020

polkadot_proposer_block_constructed_count not incrementing in branch prgn-remove-old-service-second-try #1757

Closed

coriolinus added a commit that referenced this pull request Sep 28, 2020

Revert "Remove Old Service, 2nd try (#1732)"

a8d2f1e

This reverts commit 42fbca6.

coriolinus mentioned this pull request Sep 28, 2020

Revert "Remove Old Service, 2nd try" #1758

Merged

ghost pushed a commit that referenced this pull request Sep 28, 2020

Revert "Remove Old Service, 2nd try (#1732)" (#1758)

c80f7b6

This reverts commit 42fbca6.

coriolinus mentioned this pull request Oct 2, 2020

Remove old service, 3rd try #1776

Merged

This pull request was closed.

	if !test {
	// If we're using prometheus, use a registry with a prefix of `polkadot`.
	if let Some(PrometheusConfig { registry, .. }) = config.prometheus_config.as_mut() {
	*registry = Registry::new_custom(Some("polkadot".into()), None)?;
	}
	}

	let grandpa_hard_forks = if config.chain_spec.is_kusama() && !test {
	crate::grandpa_support::kusama_hard_forks()
	} else {
	Vec::new()
	};

Comments

Conversation

coriolinus commented Sep 18, 2020

Uh oh!

Uh oh!

ordian left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bkchr commented Sep 23, 2020

Uh oh!

bkchr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bkchr Sep 24, 2020

Choose a reason for hiding this comment

Uh oh!

bkchr Sep 24, 2020

Choose a reason for hiding this comment

Uh oh!

ordian Sep 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bkchr Sep 24, 2020

Choose a reason for hiding this comment

Uh oh!

cecton Sep 26, 2020

Choose a reason for hiding this comment

Uh oh!

ordian Sep 26, 2020

Choose a reason for hiding this comment

Uh oh!

cecton Sep 28, 2020

Choose a reason for hiding this comment

Uh oh!

ordian commented Sep 24, 2020

Uh oh!

bkchr commented Sep 24, 2020

Uh oh!

bkchr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coriolinus commented Sep 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coriolinus commented Sep 28, 2020

Uh oh!

ghost commented Sep 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ordian left a comment •

edited

Loading

ordian Sep 24, 2020 •

edited

Loading

coriolinus commented Sep 28, 2020 •

edited

Loading