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

[ZMQ] append a message sequence number to every ZMQ notification #7762

Merged
merged 2 commits into from Apr 19, 2016

Conversation

Projects
None yet
6 participants
@jonasschnelli
Member

jonasschnelli commented Mar 29, 2016

Currently, ZMQ listeners cannot detect if they have missed a message.
This PR adds a sequence number to each message (each message type has its own counter).

The sequence number is the last part of the multipart zmq message to not break the API, though, we could consider breaking the API in favor of moving the sequence number to the very beginning.

Todo:

  • update release notes
@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Mar 29, 2016

Member

Concept ACK, I like adding the sequence nr as an extra part. Also ok with using 32 bit sequence numbers.

Member

laanwj commented Mar 29, 2016

Concept ACK, I like adding the sequence nr as an extra part. Also ok with using 32 bit sequence numbers.

@promag

This comment has been minimized.

Show comment
Hide comment
@promag

promag Mar 30, 2016

Member

I thought a subscriber can't miss a message.

Edit:

But it can: http://stackoverflow.com/a/15821036. So what can a subscriber do if it detects a missing message?

Member

promag commented Mar 30, 2016

I thought a subscriber can't miss a message.

Edit:

But it can: http://stackoverflow.com/a/15821036. So what can a subscriber do if it detects a missing message?

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Mar 30, 2016

Member

@promag:
I'm not sure if I would design an listening application that fully rely on getting all notifications. With this PR, you could at least detect if you got all (also check if you got the first) notifications and if not, you could poll/sync your data over RPC.

Member

jonasschnelli commented Mar 30, 2016

@promag:
I'm not sure if I would design an listening application that fully rely on getting all notifications. With this PR, you could at least detect if you got all (also check if you got the first) notifications and if not, you could poll/sync your data over RPC.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Mar 30, 2016

Member

I thought a subscriber can't miss a message.

The way we are using ZMQ, the sender will never block. This is sensible as we don't want to hang the application due to a slow client. But this means at some point one of the send queues will fill up and new messages will be discarded.

But it can: http://stackoverflow.com/a/15821036. So what can a subscriber do if it detects a missing message?

That's up to the application. Either stop with a fatal error, or re-start/re-sync. An event application usually does some synchronization at start-up, then starts processing incremental changes. After getting out of sync it can repeat the process. The important thing is being able to detect it and thus act on it.

Member

laanwj commented Mar 30, 2016

I thought a subscriber can't miss a message.

The way we are using ZMQ, the sender will never block. This is sensible as we don't want to hang the application due to a slow client. But this means at some point one of the send queues will fill up and new messages will be discarded.

But it can: http://stackoverflow.com/a/15821036. So what can a subscriber do if it detects a missing message?

That's up to the application. Either stop with a fatal error, or re-start/re-sync. An event application usually does some synchronization at start-up, then starts processing incremental changes. After getting out of sync it can repeat the process. The important thing is being able to detect it and thus act on it.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Apr 14, 2016

Member

This needs mention in doc/release-notes.md as well as doc/zmq.md (there, don't forget to specify the size and endianness of the sequence number), I think it is ready for merge otherwise.

Member

laanwj commented Apr 14, 2016

This needs mention in doc/release-notes.md as well as doc/zmq.md (there, don't forget to specify the size and endianness of the sequence number), I think it is ready for merge otherwise.

@MarcoFalke

View changes

Show outdated Hide outdated qa/rpc-tests/zmq_test.py
@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Apr 15, 2016

Member

Fixed @MarcoFalke nit.
Mentioned the change in doc/release-notes.md as well as in doc/zmq.md

Member

jonasschnelli commented Apr 15, 2016

Fixed @MarcoFalke nit.
Mentioned the change in doc/release-notes.md as well as in doc/zmq.md

@MarcoFalke

View changes

Show outdated Hide outdated qa/rpc-tests/zmq_test.py
@MarcoFalke

View changes

Show outdated Hide outdated contrib/zmq/zmq_sub.py
@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Apr 15, 2016

Member

@MarcoFalke: fixed nits.

Member

jonasschnelli commented Apr 15, 2016

@MarcoFalke: fixed nits.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Apr 19, 2016

Member

Here's a slight simplification of the code - gets rid of some duplication, as well as the _keepalive change:
laanwj@fb843df

Member

laanwj commented Apr 19, 2016

Here's a slight simplification of the code - gets rid of some duplication, as well as the _keepalive change:
laanwj@fb843df

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Apr 19, 2016

Member

@laanwj: Thanks. Much better. Stolen fb843df, squashed and force pushed.

Member

jonasschnelli commented Apr 19, 2016

@laanwj: Thanks. Much better. Stolen fb843df, squashed and force pushed.

@laanwj laanwj merged commit 0b25a9f into bitcoin:master Apr 19, 2016

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

laanwj added a commit that referenced this pull request Apr 19, 2016

Merge #7762: [ZMQ] append a message sequence number to every ZMQ noti…
…fication

0b25a9f [ZMQ] append a message sequence number to every ZMQ notification (Jonas Schnelli)
de821d5 [ZMQ] refactor message string (Jonas Schnelli)

@laanwj laanwj referenced this pull request Apr 19, 2016

Closed

zmq: mempool notifications #7753

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Apr 21, 2016

Contributor

The way we are using ZMQ, the sender will never block. This is sensible as we don't want to hang the application due to a slow client.

Could we make that parameterizable?
In certain cases I'd rather have message reliability than having the node to be guaranteed to be running in real time.

Contributor

dcousens commented Apr 21, 2016

The way we are using ZMQ, the sender will never block. This is sensible as we don't want to hang the application due to a slow client.

Could we make that parameterizable?
In certain cases I'd rather have message reliability than having the node to be guaranteed to be running in real time.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Apr 21, 2016

Member
Member

sipa commented Apr 21, 2016

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Apr 21, 2016

Member

In certain cases I'd rather have message reliability than having the node to be guaranteed to be running in real time.

Well notifications can be lost, through zmq or otherwise, for example if bitcoind needs to be restarted, or a myriad of other circumstances not under your control. At least you can detect it now:

  • sequence number goes to 0 (and wasn't at 0xffffffff) -> bitcoind restarted
  • sequence number skips a beat -> send buffer was full

Your application needs an (application dependent) way to resync anyhow. This is better than pretending to guarantee something.

Member

laanwj commented Apr 21, 2016

In certain cases I'd rather have message reliability than having the node to be guaranteed to be running in real time.

Well notifications can be lost, through zmq or otherwise, for example if bitcoind needs to be restarted, or a myriad of other circumstances not under your control. At least you can detect it now:

  • sequence number goes to 0 (and wasn't at 0xffffffff) -> bitcoind restarted
  • sequence number skips a beat -> send buffer was full

Your application needs an (application dependent) way to resync anyhow. This is better than pretending to guarantee something.

@str4d str4d referenced this pull request Jan 31, 2017

Merged

Add ZeroMQ notifications #2050

zkbot added a commit to zcash/zcash that referenced this pull request Feb 9, 2017

zkbot added a commit to zcash/zcash that referenced this pull request Feb 9, 2017

zkbot added a commit to zcash/zcash that referenced this pull request Feb 9, 2017

codablock added a commit to codablock/dash that referenced this pull request Sep 16, 2017

codablock added a commit to codablock/dash that referenced this pull request Sep 19, 2017

codablock added a commit to codablock/dash that referenced this pull request Dec 20, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment