-
Notifications
You must be signed in to change notification settings - Fork 227
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
revert(message)!: remove nack delay parameter #668
Conversation
callmehiphop
commented
Jun 24, 2019
•
edited
Loading
edited
- Tests and linter pass
- Code coverage does not decrease (if any source code was changed)
- Appropriate docs were updated (if necessary)
Codecov Report
@@ Coverage Diff @@
## master #668 +/- ##
==========================================
- Coverage 97.79% 97.79% -0.01%
==========================================
Files 14 14
Lines 861 860 -1
Branches 179 179
==========================================
- Hits 842 841 -1
Misses 2 2
Partials 17 17
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #668 +/- ##
==========================================
- Coverage 97.79% 97.79% -0.01%
==========================================
Files 14 14
Lines 861 860 -1
Branches 179 179
==========================================
- Hits 842 841 -1
Misses 2 2
Partials 17 17
Continue to review full report at Codecov.
|
@JustinBeckwith @bcoe @callmehiphop can one of you please explain why this was reverted / removed? I tried searching issues to figure out why on my own, but couldn't find anything. I believe our team utilized this because the global pub / sub delay setting wasn't working properly. Has that been fixed now and why this is no longer needed? Even if the global setting has been fixed, wouldn't there still be value in a lower level setting for overriding or performing and incremental backoff / increase? Thanks in advance! 🙏 |
@manovotny I believe this came at the request of the PubSub team. @anguillanneuf do you have any insights here? |
Yes ... not allowing a "retry delay" is very frustrating on the app side. If the app isn't ready to handle the message now, why would it be reasonable to think it will be ready in 10 milliseconds or so? It's not like the low level API doesn't have this capability, hiding it from the "friendly" interface is unfortunate. |
@mgabeler-lee-6rs technically the API doesn't offer such functionality, the client was just using the API in a way the backend team never intended, hence why they asked us to remove it. The team definitely sees value in this kind of functionality, so that's not to say it won't resurface in some form one day. I know that using |
we tried |
@mgabeler-lee-6rs just to clarify, you are saying that you get stuck by the flow control limits since you're waiting for all the pending nacks to fire off to allow new messages to come in? |
right -- if we only have the memory budget for X messages, and we need to have 10*X messages "in progress" before we see a message we need as a pre-requisite to one of the first X messages, we are up the proverbial creek. Edit: And also we found empirically that we tend to get the just-nack'd messages back more than we get the other "later" messages that we're wanting to process first. But if we can tell those first 9*X messages to come back in a few seconds or maybe a minute, we can blast through and handle the prerequisite messages no problem. This is one of several things we learned in a painful incident that involved a backlog of over eight million messages on a single subscription one day (the other being that nacking messages at all is hazardous to the subscription's health). It is also just quite icky to have to use application memory in our kubernetes cluster to do message queueing on behalf of ... a message queuing system. |
@anguillanneuf @kamalaboulhosn have you seen this issue in any of the other clients? |
It seems like this is trying to use nack delay as a means to get around the fact that Cloud Pub/Sub does not currently support ordering. Using the delay as a means to overcome the lack of ordering would be considered an anti pattern. If this is a requirement for you, I would recommend pinning to a version of the library that supports it until we provide a solution that better meets your needs. |
@kamalaboulhosn Indeed ... for newer code we have found better patterns to avoid this issue. And pinning at |