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

Remove with_gossip#2500

Closed
tomaka wants to merge 11 commits into
paritytech:masterfrom
tomaka:net-threads-3
Closed

Remove with_gossip#2500
tomaka wants to merge 11 commits into
paritytech:masterfrom
tomaka:net-threads-3

Conversation

@tomaka

@tomaka tomaka commented May 7, 2019

Copy link
Copy Markdown
Contributor

Based on top of #2497. Only the last commit is relevant.

Removes the Service::with_gossip method and turn its usage around the code into individual method calls.

@tomaka tomaka added the A0-please_review Pull request needs code review. label May 7, 2019
@rphmeier

rphmeier commented May 7, 2019

Copy link
Copy Markdown
Contributor

Also needs a PR in Polkadot when this is merged.

@tomaka tomaka requested a review from arkpar May 7, 2019 17:08
@tomaka tomaka added the I7-refactor Code needs refactoring. label May 7, 2019
@tomaka tomaka requested a review from gterzian May 7, 2019 17:08
@gterzian

gterzian commented May 8, 2019

Copy link
Copy Markdown
Contributor

What's the rationale for removing the generic with_gossip method with dedicated methods? From the discussion me and @rphmeier had over at #1340 (comment) I took away that we didn't want to bloat the API with too many specific methods...

@tomaka

tomaka commented May 8, 2019

Copy link
Copy Markdown
Contributor Author

Just like the other PRs, I'm trying to make the API less dependant on the threading strategy. To me, passing a closure to with_gossip that gets executed later is strictly worse than doing a "flat call" to a method that does what your closure contains.

@gterzian

gterzian commented May 8, 2019

Copy link
Copy Markdown
Contributor

I fail to see how the last commit changes the situation with regards to the dependence on the threading strategy.

We're trading a single with_gossip api endpoint for four specific ones. And those new methods still result in a message being sent across, which will run an "operation" on another task.

The only difference is that every operation we come up with later will require a new method, and message, whereas with_gossip allowed the client to execute a generic operation using gossip.

If what you mean is that you want the "operation" corresponding to the method call to execute immediately, we could still keep with_gossip, and make it a blocking call using a bounded sender, or a lock around gossip.

@rphmeier

rphmeier commented May 8, 2019

Copy link
Copy Markdown
Contributor

Yeah, I still think with_gossip is still much more robust and we don't have to deal with service API bloat.

@tomaka

tomaka commented May 8, 2019

Copy link
Copy Markdown
Contributor Author

Well, ok. I don't want to spend energy convincing people, so will reopen at a later time on top of other changes when the necessity of that change becomes a bit more obvious.

@tomaka tomaka closed this May 8, 2019
@tomaka tomaka mentioned this pull request Jun 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A0-please_review Pull request needs code review. I7-refactor Code needs refactoring.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants