Skip to content
This repository has been archived by the owner on Jun 26, 2023. It is now read-only.

Better feedback when using publish #96

Closed
D4nte opened this issue May 17, 2021 · 5 comments
Closed

Better feedback when using publish #96

D4nte opened this issue May 17, 2021 · 5 comments
Assignees
Labels
exp/expert Having worked on the specific codebase is important need/analysis Needs further analysis before proceeding need/community-input Needs input from the wider community P3 Low: Not priority right now

Comments

@D4nte
Copy link

D4nte commented May 17, 2021

The Publish method is currently defined to return Promise<void>.

However, from experience, there can be a number of reasons for which a message is not sent to any peer.

The current publish method seems to be a shot in the dark.

I would propose to update the interface to return some information on the attempt made by the method to send messages. At this stage I would be interesting to know if the method successfully sent the message to at least one peer so returning Promise<boolean> could be enough.

However, with a mindset of extensibility, returning the following could give more options to API consumers:

Promise<{
	success: PeerId[],
	fail: PeerId[]
}>

With success the peers for which it seems that the message was sent, ie, no error was raised from the stream, etc.
fail the peers that were selected for publishing but some error was returned.

Note: Maybe success is not the right terminology as I understand messages are not ack'd.
attempted and immediateFail could be an alternative naming.

I assume that'd I need to play around with the code first to confirm that said API would make sense.

If we look other implementations such as rust-libp2p, the publish method returns a result (back to the boolean) where, for example, an error is returned if no peers were selected:

https://github.com/libp2p/rust-libp2p/blob/c5bcada2c2a1e34b8e39b885f10ed7c2d00e8373/protocols/gossipsub/src/behaviour.rs#L681

Thoughts?

@D4nte D4nte changed the title Better feedback on publishing Better feedback when using publish May 17, 2021
@vasco-santos
Copy link
Member

cc @wemeetagain

@lidel lidel added exp/expert Having worked on the specific codebase is important need/analysis Needs further analysis before proceeding P3 Low: Not priority right now need/community-input Needs input from the wider community labels May 17, 2021
@vasco-santos
Copy link
Member

At the very least, we should probably do what rust does: If we do not have peers to send messages throw an error.

Regarding the actual counters, I don't think we can get to a good solution. Two things can happen:

  • Peers can ignore the message received
  • Peers can forward the message to other peers

With this in mind, IMHO we will not return reliable information. But I would like @wemeetagain input on this one

@wemeetagain
Copy link
Member

Hey, late reply here..

Another odd thing that may happen in gossipsub: a message is published, but not sent to any peers. Then more peers join and the message gossiped and sent to those peers.

I guess that can't happen in the rust implementation, but I think it happens in this and the golang implementation.

@wemeetagain
Copy link
Member

Related PR #119

@wemeetagain
Copy link
Member

resolved with #199

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
exp/expert Having worked on the specific codebase is important need/analysis Needs further analysis before proceeding need/community-input Needs input from the wider community P3 Low: Not priority right now
Projects
None yet
Development

No branches or pull requests

4 participants