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

nsqd: set memoryMsgChan as nil when --mem-queue-size=0 #1159

Merged
merged 1 commit into from
Sep 7, 2019

Conversation

bitpeng
Copy link
Contributor

@bitpeng bitpeng commented Jun 5, 2019

Note: generally, we intend to save all the message in the disk-queue when set -mem-queue-size=0 option, but make(chan *Message, 0) just create a unbuffered chan which can also send/receive message as well.

the Go channel spec dictates that nil channel operations (read or write) in a select are skipped, we set memoryMsgChan as not nil noly when mem-queue-size > 0 as shown in diskqueue.go

@mreiferson mreiferson changed the title set memoryMsgChan as nil when -mem-queue-size=0 nsqd: set memoryMsgChan as nil when --mem-queue-size=0 Jun 5, 2019
@bitpeng
Copy link
Contributor Author

bitpeng commented Jun 25, 2019

Thanks for your warmly, patient answer, but in my opinion it should give the user an option to have a chance to save all the messages on the disk-queue. Do you think so? @mreiferson

@ploxiln
Copy link
Member

ploxiln commented Jun 25, 2019

This isn't really enough to "guarantee" that, though.

The disk segments are not necessarily synced after every message, so the message might still be buffered in the page cache and not written to the physical disk, though you could set --sync-every to 1 to address that, but it would hurt performance, quite significantly.

But more importantly, messages are taken out of the disk queue while they are in flight, and exist in the in-memory in-flight pqueue. If the server crashes hard at this point, they will probably be lost. They may still exist in some disk segment file, but the metadata will say that they are in an already-consumed part of that disk segment file, and they won't be re-played. (And they might have been in a segment file that was finished and cleaned-up).

To get what you're going for, you need something like #625

@LiPL2017
Copy link

LiPL2017 commented Sep 6, 2019

Without considering the network factors,It's very usefull when i need to keep message order.

@ploxiln
Copy link
Member

ploxiln commented Sep 6, 2019

OK ... I'm convinced this change is a good idea. There are still serious limitations to the "disk-queue-only" mode which make me skeptical of its value, but anyone who configured --mem-queue-size=0 very likely wanted and expected every message to go into the diskqueue.

do not use unbuffered chan for in-memory queue when size=0
because user intended all messages to go through backend queue (disk)
@ploxiln
Copy link
Member

ploxiln commented Sep 6, 2019

I took the liberty of changing the code and comment style. Local testing with --mem-queue-size=0 shows that nsqd does not lock up or spin or anything crazy. I'll merge later today if there are no objections.

@ploxiln ploxiln merged commit 470346f into nsqio:master Sep 7, 2019
ploxiln added a commit to ploxiln/nsq that referenced this pull request Sep 15, 2021
In b3b29b7 / nsqio#1159 unbuffered
memoryMsgChan were replaced with nil chan for both Topic and Channel
(this only happens if mem-queue-size=0). This inadvertently made
deferred publish never work (with mem-queue-size=0), because some
metadata is lost when messages go through the "backend" disk queue,
but previously messages could go immediately through the topic's
unbuffered chan with their metadata intact, to then be added to each
channel's deferred pqueue. This partial revert brings this part back.

Granted, mem-queue-size=0 never worked very well, in many respects.
For this functionality, at low message publish rates, it seems there
was rarely a problem. But at high message publish rates it was always
likely that some messages would hit the topic disk queue and lose
the deferred time metadata. This potential problem remains.

In fact, the motivation for fully disabling the memoryMsgQueue for
mem-queue-size=0 was to avoid excessively shuffling message order,
which would happen if some messages jump instantly through the
memory-queue while others take the longer way through the disk-queue.
That will be likely again. But the users who noticed this probably
did not use publish-deferred functionality!

Additional fixes would be appropriate, but this is a quick-fix to
restore previous useful behavior for mem-queue-size=0.
ploxiln added a commit to ploxiln/nsq that referenced this pull request Aug 2, 2022
In b3b29b7 / nsqio#1159 unbuffered
memoryMsgChan were replaced with nil chan for both Topic and Channel
if mem-queue-size=0. This inadvertently made deferred publish never
work (with mem-queue-size=0), because some metadata is lost when
messages go through the "backend" disk queue, instead of immediately
going through the topic memory chan to the channels' deferred pqueues.

The motivation for fully disabling the memoryMsgQueue for
mem-queue-size=0 was to avoid excessively shuffling message order,
which would happen if some messages jump instantly through the
memory-queue while others take the longer way through the disk-queue.
So, only allow using the memory queue in this case if really needed,
for deferred messages, or for ephemeral topic or channel which just
lose all messages if they all go through the no-op backend queue.

Granted, mem-queue-size=0 never worked very well, in many respects.
qaqhy pushed a commit to qaqhy/nsq that referenced this pull request Jul 29, 2024
In b3b29b7 / nsqio#1159 unbuffered
memoryMsgChan were replaced with nil chan for both Topic and Channel
if mem-queue-size=0. This inadvertently made deferred publish never
work (with mem-queue-size=0), because some metadata is lost when
messages go through the "backend" disk queue, instead of immediately
going through the topic memory chan to the channels' deferred pqueues.

The motivation for fully disabling the memoryMsgQueue for
mem-queue-size=0 was to avoid excessively shuffling message order,
which would happen if some messages jump instantly through the
memory-queue while others take the longer way through the disk-queue.
So, only allow using the memory queue in this case if really needed,
for deferred messages, or for ephemeral topic or channel which just
lose all messages if they all go through the no-op backend queue.

Granted, mem-queue-size=0 never worked very well, in many respects.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants