search

nsqio / go-nsq

The official Go package for NSQ
MIT License
2.59k stars 444 forks source link

consumer: update RDY when setting MaxInFlight to 0 (pause consumer) #222

Closed mscso closed 5 years ago

mscso commented 6 years ago

When trying to pause the consumer via MaxInFlight 0, updateRDY is not called as maybeUpdateRDY checks for "count > 0". This PR removes that check from the condition.

Now I'm not sure if there's a legitimate reason for not updating RDY when MaxInFlight is 0, but right now the code breaks the "pause consumer" use case.

mreiferson commented 6 years ago

Good catch! I think we need to approach this a little differently, perhaps by special casing 0 in ChangeMaxInFlight() and making sure to check/clear out any backoff state?

mscso commented 6 years ago

Not sure which special casing you mean - I can see the comment "For example, ChangeMaxInFlight(0) would pause message flow" - but no actual check for it?

mreiferson commented 6 years ago

Exactly, I think ChangeMaxInFlight should bypass normal RDY handling and set connections to 0, but it needs to also account for backoff state on any of those connections.

mscso commented 6 years ago

Ah understood and I guess that makes sense. Personally I'd chose to not add additional code paths and simplify instead - but that's your call :)

Let me know if I can help.

mreiferson commented 5 years ago

gonna fix this in #249, you ended up being on the right track after all, thanks!