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

consumer: order-of-operations for messagesReceived #227

Closed
wants to merge 1 commit into from
Closed

consumer: order-of-operations for messagesReceived #227

wants to merge 1 commit into from

Conversation

qianlnk
Copy link

@qianlnk qianlnk commented Apr 28, 2018

1、messagesReceived should be add after message had transport to message channel.

2、 add Consume method,use Consume to get message channel if someone want to custom ack or nack.

@mreiferson
Copy link
Member

@qianlnk thanks for the PR, but I don't actually understand the bug. Also, for numerous reasons, it's not safe to expose the go channel over which messages are delivered, so we wouldn't merge that API addition.

@qianlnk
Copy link
Author

qianlnk commented May 1, 2018

@mreiferson We can't receive the message from message channel until the message transfer to message channel(because the message channel may be blocked). I hold the opinion that count of messagesReceived should be put after the message transfer to message channel. as for the user, messagesReceived must be equal to the number received from message handler.

@ploxiln
Copy link
Member

ploxiln commented May 1, 2018

You propose to not increment messagesReceived until the handler starts processing the message. I think the original intention is to count the message received as soon as the process receives it from nsqd (and I think this is even more important for totalRdyCount).

@ploxiln
Copy link
Member

ploxiln commented May 2, 2018

hmm ... I think it might be possible that some messages could be received by the consumer process from nsqd, just after Stop() is called and incomingMessages is closed, and we could possibly handle it more elegantly, though it's a tricky distributed timing problem ... what you could do is call ChangeMaxInFlight(0) and wait a couple seconds, then call Stop(), and in that case you should get consistent numbers, and no dropped messages

@mreiferson mreiferson added the bug label Nov 11, 2018
@mreiferson mreiferson changed the title fix messagesReceived bug, add method Consume consumer: order-of-operations for messagesReceived Nov 11, 2018
@mreiferson mreiferson closed this Nov 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants