Skip to content

Synchronous producer - Handle acknowledgements of sent PUB/MPUB/DPUB messages#52

Open
Soulou wants to merge 15 commits intowistia:masterfrom
Scalingo:feature/51/acknowledgement
Open

Synchronous producer - Handle acknowledgements of sent PUB/MPUB/DPUB messages#52
Soulou wants to merge 15 commits intowistia:masterfrom
Scalingo:feature/51/acknowledgement

Conversation

@Soulou
Copy link
Contributor

@Soulou Soulou commented Jan 17, 2019

The goal of this PR is to update the library to ensure that an exception is raised if an error happens when a message is sent to NSQd.

This is a work in progress, all the inputs are welcomed.

Related #51

@Soulou
Copy link
Contributor Author

Soulou commented Jan 17, 2019

The first step has been to gather read/write operations in a single thread in order to share information between both operations in an easier way.

@Soulou
Copy link
Contributor Author

Soulou commented Jan 17, 2019

Second step, not sure it's the best implementation but a SizableQueue of 1 item is created for each message and publishing Thread is waiting for the response to be received. I got inspired from the go implementation which is handling it this way.

@Soulou
Copy link
Contributor Author

Soulou commented Jan 18, 2019

I think the PR is ready, the API hasn't changed, the behavior either, but the internals have slightly been updated as there is only one read/write loop based on a select.

The code is ready for review. All the old specs are passing correctly.

@Soulou Soulou force-pushed the feature/51/acknowledgement branch from 35dd4b2 to f371fea Compare January 18, 2019 14:58
@Soulou Soulou changed the title WiP - Handle message acknowledgement of sent messages Synchronous producer - Handle acknowledgements of sent PUB/MPUB messages Jan 18, 2019
@Soulou
Copy link
Contributor Author

Soulou commented Jan 21, 2019

@bschwartz a small follow up to get your input on this.

@Soulou Soulou force-pushed the feature/51/acknowledgement branch from 1e2530f to 69a5a48 Compare January 27, 2019 23:05
@Soulou Soulou force-pushed the feature/51/acknowledgement branch from 69a5a48 to fe8f9e9 Compare January 27, 2019 23:29
@Soulou Soulou force-pushed the feature/51/acknowledgement branch from db2bb92 to d869114 Compare January 29, 2019 11:48
This commit has been inspired greatly by what is done in the official
golang driver.

To follow the recommandations of the NSQ team, the producer is not able
to get initialized with nsqlookupd URL, a producer is connecting to
only one NSQd instance, not several.

- The producer is keeping a list of transactions to wait for the return
value of NSQd.
- A new class `NsqdsProducer` can be initialized with multiple NSQds
addresses and will apply a strategy to write messages:
  - `Nsq::NsqdsProducer::STRATEGY_FAILOVER` Always send on the same
nsqd except if not available, then get to the next one
  - `Nsq::NsqdsProducer::STRATEGY_ROUND_ROBIN` Apply round robin over
the different available nsqds
@Soulou Soulou force-pushed the feature/51/acknowledgement branch from 2571e34 to 198ddd5 Compare January 29, 2019 22:01
@Soulou
Copy link
Contributor Author

Soulou commented Jan 29, 2019

There is a race condition on the failing tests.

if SUB is sent too early in nsq starting process, it is refused with E_INVALID invalid SUB during init state
As it's not detected, the consumer is not registering on the topic and not getting the message.

The bug exists on master too, all the other specs are passing

@bschwartz
Copy link
Member

This is looking good @Soulou. I should have time Friday for a more in depth review. In the meantime, do you think you could also update the README to explain the new functionality?

Also, if you have some ideas of how to fix the spec issue that you mentioned also exists on master, that would be great!

@Soulou
Copy link
Contributor Author

Soulou commented Jan 30, 2019

Thanks @bschwartz I'll update the README and try to fix the spec,

@Soulou Soulou changed the title Synchronous producer - Handle acknowledgements of sent PUB/MPUB messages Synchronous producer - Handle acknowledgements of sent PUB/MPUB/DPUB messages Jan 30, 2019
@Soulou
Copy link
Contributor Author

Soulou commented Jan 30, 2019

For info, this PR is already running in our production environment, so far so good.

@Soulou
Copy link
Contributor Author

Soulou commented Jan 31, 2019

@bschwartz done :)

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.

2 participants