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

sc-network: expose add_to_priority_group and `remove_from_priority_… #7247

Merged
9 commits merged into from
Oct 9, 2020
Merged

sc-network: expose add_to_priority_group and `remove_from_priority_… #7247

9 commits merged into from
Oct 9, 2020

Conversation

@ordian
Copy link

@ordian ordian commented Oct 1, 2020 •
edited
Loading

A follow up to #3376 (comment).
Needed for paritytech/polkadot#1763.

polkadot companion: paritytech/polkadot#1800

@ordian ordian 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. labels Oct 1, 2020
@ordian ordian requested a review from mxinden October 1, 2020 13:43
ordian added 2 commits October 1, 2020 15:44
@ordian
sc-network: fix a typo
f999356
@ordian
Merge branch 'master' into ao-network-expose-incremental-priority-gro…
d0f0565 
…up-updates

* master:
  client/cli: Update to fdlimit 0.2.1 (#7249)
  Fix typo cranelift_wasm (#7248)
  Remove hack around secondary connections handshakes (#7246)
mxinden
mxinden reviewed Oct 9, 2020
View reviewed changes
Copy link
Contributor

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Sorry for the late review @ordian.

client/network/src/service.rs Outdated
///
/// Returns an `Err` if one of the given addresses is invalid or contains an
/// invalid peer ID (which includes the local peer ID).
pub fn add_to_priority_group(&self, group_id: String, peers: HashSet<Multiaddr>) -> Result<(), String> {
Copy link
Contributor

@mxinden mxinden Oct 9, 2020

Choose a reason for hiding this comment

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

Suggested change
pub fn add_to_priority_group(&self, group_id: String, peers: HashSet<Multiaddr>) -> Result<(), String> {
pub async fn add_to_priority_group(&self, group_id: String, peers: HashSet<Multiaddr>) -> Result<(), String> {

Declaring this method as async would grant us a greater implementation flexibility in the future, e.g. a bounded channel between NetworkService and NetworkWorker.

@tomaka what do you think of requiring all new methods on NetworkService to be able to yield?

@ordian Are you able to await this method within Polkadot?

Copy link
Author

@ordian ordian Oct 9, 2020

Choose a reason for hiding this comment

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

yes, awaiting is fine, but would we need to change all three methods then (set_, add_, remove_)?

Copy link
Author

@ordian ordian Oct 9, 2020

Choose a reason for hiding this comment

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

Done in b1c28b2.

Copy link
Contributor

@tomaka tomaka Oct 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

👍 As eventually it'd be nice if the queue from the network service to the worker was bounded as well.

ordian and others added 3 commits October 9, 2020 11:59
@ordian
Merge branch 'master' into ao-network-expose-incremental-priority-gro…
f089aaf 
…up-updates

* master:
  Async keystore + Authority-Discovery async/await (#7000)
  Fixes logging of target names with dashes (#7281)
  seal: Add automated weights for contract API calls (#7017)
  add ss58 id for nodle (#7279)
  Refactor CurrencyToVote (#6896)
  bump-allocator: document & poison (#7277)
  Reset flaming fir network (#7274)
  reschedule (#6860)
  Drop system cache for trie benchmarks (#7242)
  client: improve log formatting (#7272)
  Rework `InspectState` (#7271)
  SystemOrigin trait (#7226)
  Update ss58 registry for Dock network (#7263)
  .maintain/monitoring: Add alert when continuous task ends (#7250)
  Rename `TRANSACTION_VERSION` to `EXTRINSIC_VERSION` (#7258)
  Split block announce processing into two parts (#6958)
  Fix offchain election to respect the weight (#7215)
@ordian @mxinden
Update client/network/src/service.rs
c751e29 
Co-authored-by: Max Inden <[email protected]>
@ordian
s/parse_multiaddr/split_multiaddr_and_peer_id/g
3ea90f2
ordian added 2 commits October 9, 2020 12:18
@ordian
Merge branch 'ao-network-expose-incremental-priority-group-updates' o…
98bbafa 
…f github.com:paritytech/substrate into ao-network-expose-incremental-priority-group-updates

* 'ao-network-expose-incremental-priority-group-updates' of github.com:paritytech/substrate:
  Update client/network/src/service.rs
@ordian
sc-network: mark new functions as async and add comments
b1c28b2
@ordian ordian mentioned this pull request Oct 9, 2020
Merged
mxinden
mxinden approved these changes Oct 9, 2020
View reviewed changes
Copy link
Contributor

@mxinden mxinden 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 to me. Small suggestion. I am fine either way.

@ordian
Copy link
Author

ordian commented Oct 9, 2020

bot merge

@ghost
Copy link

ghost commented Oct 9, 2020

Waiting for commit status.

@ghost
Copy link

ghost commented Oct 9, 2020

Merge failed: "Required status check \"continuous-integration/gitlab-check-labels\" is pending."

@ordian
Copy link
Author

ordian commented Oct 9, 2020

bot merge

@ghost
Copy link

ghost commented Oct 9, 2020

Trying merge.

@ghost ghost merged commit 2f3b20b into master Oct 9, 2020
@ghost ghost deleted the ao-network-expose-incremental-priority-group-updates branch October 9, 2020 14:12
@mxinden mxinden mentioned this pull request Oct 19, 2020
Merged
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@tomaka tomaka tomaka approved these changes

@mxinden mxinden mxinden 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.

4 participants

@ordian @tomaka @mxinden