Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

enhancement: hub protocol #1637

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

apokriff
Copy link
Contributor

@apokriff apokriff commented Dec 26, 2022

Cogent Embedded created new protocol (hub) and would like to share it.

Hub protocol looks like bus, but it is implemented with guarantees for send / receive messages. For our purposes we made it compatible with pair0

Add hub protocol.
Protocol with star topology and
send/receive guarantees.

Protocol is compatible with pair0
@gdamore
Copy link
Contributor

gdamore commented Feb 5, 2023

This looks quite good.

Questions/concerns:

  1. It looks like this isn't really "reliable" -- in fact I was very concerned about the claim that it was, because, it's impossible to have reliable multicast unless one has infinite buffering space or is willing to stop the entire network if one peer is slow (or hung). Instead you've done the reasonable thing, which is provide a buffer and and drop messages when the buffer is full. In that regard, this doesn't look any more reliable than e.g. pub/sub or bus or star.
  2. Given that - is there a reason you didn't use bus?
  3. Is there a reason you chose to make this compatible with pair0 instead of pair1?

@gdamore
Copy link
Contributor

gdamore commented Feb 5, 2023

It would also be good to understand why this didn't run the test suites. You did the work to add one... it should have run...

@apokriff
Copy link
Contributor Author

apokriff commented Feb 8, 2023

Given that - is there a reason you didn't use bus?
The main reason for not using bus is that bus drop messages silently. In our case it is required to get error if message is not sent or received.

Is there a reason you chose to make this compatible with pair0 instead of pair1?
The only reason is compatibility with old versions. Previously we use pair0, but now we have to support multiple "clients" in parallel. We receive on our server commands from any client and send answers to all of them. Server sends commands to clients as well. Actually it is close to array of pairs except server commands should be sent to everyone

@gdamore
Copy link
Contributor

gdamore commented Feb 8, 2023

Given that - is there a reason you didn't use bus?
The main reason for not using bus is that bus drop messages silently. In our case it is required to get error if message is not sent or received.

Is there a reason you chose to make this compatible with pair0 instead of pair1?
The only reason is compatibility with old versions. Previously we use pair0, but now we have to support multiple "clients" in parallel. We receive on our server commands from any client and send answers to all of them. Server sends commands to clients as well. Actually it is close to array of pairs except server commands should be sent to everyone

So it turns out that this will also drop messages if the queue becomes too full. The main difference is that compared to BUS this is a bit more tolerant of bursts -- BUS will drop if TCP is briefly not keeping up, whereas HUB will continue to queue for a bit until the queue itself fills up.

The documentation should make this clear.

And in retrospect, I wonder if this need might not have been better satisfied by changing BUS to have a tunable queue depth that could be used instead of dropping when the underlying transport pipe is too busy.

I want to think about this and look at the actual code a little bit more before I merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants