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

split NetworkBridge into two subsystems #5616

Merged
paritytech-processbot merged 29 commits into from
Jul 12, 2022
Merged

split NetworkBridge into two subsystems #5616

paritytech-processbot merged 29 commits into from
Jul 12, 2022

Conversation

@drahnr
Copy link
Contributor

@drahnr drahnr commented May 31, 2022 •
edited
Loading

Split NetworkBridgeSubsystem into In and Out parts, where In and Out are respective to the network. So In handles incoming network messages, Out handles messages that are meant to go to the network. NewGossipTopology should be moved into GossipSupportSubsystem, it currently lives in In, where it does not belong.

Depends on #5611

@github-actions github-actions bot added the A0-please_review Pull request needs code review. label May 31, 2022
@drahnr drahnr added 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. labels May 31, 2022
@drahnr drahnr mentioned this pull request Jun 1, 2022
Merged
4 tasks
@drahnr drahnr requested review from a team and chevdor as code owners June 1, 2022 15:46
@drahnr drahnr mentioned this pull request Jun 2, 2022
Closed
@drahnr drahnr requested review from ordian and sandreim June 3, 2022 12:07
@drahnr drahnr force-pushed the bernhard-split-network-bridge-into-two-subsystems branch from fca0a21 to df0b74e Compare June 3, 2022 12:08
@drahnr drahnr changed the base branch from bernhard-network-bridge-refactor to master June 3, 2022 12:09
sandreim
sandreim reviewed Jun 6, 2022
View reviewed changes
@drahnr drahnr force-pushed the bernhard-split-network-bridge-into-two-subsystems branch from df0b74e to 3f02edc Compare June 6, 2022 22:39
@rphmeier rphmeier mentioned this pull request Jun 8, 2022
Open
@drahnr drahnr requested review from eskimor and sandreim June 13, 2022 21:51
ordian
ordian reviewed Jun 16, 2022
View reviewed changes
Copy link

@ordian ordian left a comment

Choose a reason for hiding this comment

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

Didn't look deeply since it's mostly moving code around, but IMHO it warrants a burn-in on Versi at least to be on the safer side.

@drahnr drahnr force-pushed the bernhard-split-network-bridge-into-two-subsystems branch from ccf1009 to 3edac5a Compare June 20, 2022 15:21
drahnr and others added 10 commits June 20, 2022 17:50
@drahnr
cleanup
b8004a5
@drahnr
remove dead code
1762559
@drahnr
remove sync bounds again
653c461
@drahnr
wire up shared state
9ba9173
@drahnr
deal with some FIXMEs
5019318
@drahnr @sandreim
use distinct tags
ad37193 
Co-authored-by: Andrei Sandu <[email protected]>
@drahnr @sandreim
use tag
683b978 
Co-authored-by: Andrei Sandu <[email protected]>
@drahnr
address naming
c7337c2 
tx and rx are common in networking and also have an implicit meaning regarding networking
compared to incoming and outgoing which are already used with subsystems themselvesq
@drahnr
remove unused sync oracle
079a28c
@drahnr
remove unneeded state
6109018
@drahnr drahnr force-pushed the bernhard-split-network-bridge-into-two-subsystems branch from 940ad6b to 6109018 Compare June 20, 2022 15:50
@drahnr drahnr requested a review from ordian June 22, 2022 15:09
@ordian
Copy link

ordian commented Jun 22, 2022 

thread 'benchmark_block_works' panicked at 'assertion failed: common::wait_for(&mut cmd, 30).map(|x| x.success()).unwrap_or_default()', tests/benchmark_block_works.rs:72:5

This test failed twice. Don't know if this is coincidence. Starting a burn-in process on Versi.

@ordian
Copy link

ordian commented Jun 22, 2022 

main polkadot_service: Failed to init overseer: Duplicate metrics collector registration attempted

ordian
ordian reviewed Jun 22, 2022
View reviewed changes
@drahnr
Copy link
Contributor Author

drahnr commented Jun 23, 2022 

running 1 test
test benchmark_block_works has been running for over 60 seconds
2022-06-23 11:19:42 Essential task `txpool-background` failed. Shutting down service.    
2022-06-23 11:19:42 ParentBlockRandomness did not provide entropy    
2022-06-23 11:19:42 Block 1 with     2 tx used 200.88% of its weight (    18,041,793 of      8,981,574 ns) - OVER WEIGHT!    
2022-06-23 11:20:51 Essential task `txpool-background` failed. Shutting down service.    
2022-06-23 11:20:51 ParentBlockRandomness did not provide entropy    
2022-06-23 11:20:51 Block 1 with     2 tx used 190.34% of its weight (    18,785,974 of      9,869,799 ns) - OVER WEIGHT!    
2022-06-23 11:21:44 Essential task `txpool-background` failed. Shutting down service.    
2022-06-23 11:21:44 ParentBlockRandomness did not provide entropy    
2022-06-23 11:21:44 Block 1 with     2 tx used 201.04% of its weight (    17,653,927 of      8,781,241 ns) - OVER WEIGHT!    
test benchmark_block_works ... ok

@drahnr drahnr requested a review from ordian June 23, 2022 09:41
ordian
ordian approved these changes Jun 23, 2022
View reviewed changes
Copy link

@ordian ordian left a comment

Choose a reason for hiding this comment

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

Versi burn-in seems fine.

It's unfortunate we had to share the metrics because of the view updates sent in rx which increment notification sent metric used by tx :/

ordian
ordian reviewed Jun 23, 2022
View reviewed changes
@drahnr
Copy link
Contributor Author

drahnr commented Jul 12, 2022

Gentle ping :)

Merge branch 'master' into bernhard-split-network-bridge-into-two-sub…
687ae9a 
…systems

* master: (39 commits)
  Implement prune only stagnant check mode (#5761)
  Bump chevdor/srtool-actions from 0.4.0 to 0.5.0 (#5769)
  Update release-99_bot.yml (#5764)
  Version bump to v0.9.26/9260 (#5755)
  Rename `extra_constant` (#5749)
  Bump wasmtime from 0.38.0 to 0.38.1 (#5741)
  Several small fixes to the templates of the release notes (#5756)
  Backports from 0.9.25 (#5754)
  Remove bridges subtree (#5752)
  [Zombienet] test deregister validator (#5718)
  Fix regexp to find the repo name (#5751)
  Limit stagnant checks to a certain amount of entries (#5742)
  fix(staking miner): check latest state in solution (#5744)
  staking-miner: CLI flag delay solution x secs (#5734)
  backport minimum weight to fee to master (#5739)
  Bump quote from 1.0.19 to 1.0.20 (#5736)
  zombienet: try to fix parachains upgrade test (#5724)
  Update dependencies (companion for substrate#11722) (#5731)
  Update metric name and doc (#5716)
  Bump reqwest from 0.11.10 to 0.11.11 (#5732)
  ...
@ordian
Copy link

ordian commented Jul 12, 2022

bot merge

@paritytech-processbot paritytech-processbot bot merged commit ffed273 into master Jul 12, 2022
@paritytech-processbot paritytech-processbot bot deleted the bernhard-split-network-bridge-into-two-subsystems branch July 12, 2022 16:22
ordian pushed a commit that referenced this pull request Jul 22, 2022
Merge branch 'master' into ao-malus-delay-finality
c31de45 
* master:
  westend xcm: collectives parachain is trusted teleporter (#5798)
  Cleanup light client leftovers (#5794)
  Fix benchmarking tests (#5791)
  allow re-use and avoid compiling kusama parachain code (#5792)
  Introduce async runtime calling trait for runtime-api subsystem (#5782)
  add `Extrinsic Ordering` check that runs against a local reference node (#5790)
  Co #11456: Expose `benchmark extrinsic` command (#5620)
  `staking-miner`:  Add handling of `SIGTERM`, `SIGKILL`, `SIGQUIT` and `SIGINT` (#5780)
  Zombienet: paritydb test (#5310)
  Fix Typo (#5766)
  Fix Core Version display in the release notes (#5781)
  companion for new pools reward scheme (#5757)
  fix disable-runtime-api feature flag (#5773)
  split NetworkBridge into two subsystems (#5616)
  Implement prune only stagnant check mode (#5761)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@chevdor chevdor Awaiting requested review from chevdor

@eskimor eskimor Awaiting requested review from eskimor

2 more reviewers

@ordian ordian ordian approved these changes

@sandreim sandreim sandreim approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@drahnr @ordian @sandreim