-
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
Send acknowledge/modifyAckDeadline requests in chunks #65
Send acknowledge/modifyAckDeadline requests in chunks #65
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
Codecov Report
@@ Coverage Diff @@
## master #65 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 8 8
Lines 845 849 +4
=====================================
+ Hits 845 849 +4
Continue to review full report at Codecov.
|
CLAs look good, thanks! |
I'm happy to add test-cases to cover the chunking once we agree on an implementation. |
@ctavan overall I think this LGTM, did this solve your issue? Any thoughts here @stephenplusplus? |
Looks okay to me. Did anyone try the system tests? |
@callmehiphop @stephenplusplus I have not seen the error reported in #62 since running with this patched code while I was getting the error on the majority of service restarts before: So I'm pretty confident that it does what it's supposed to :). Also, we've been running this in production for the last 4 days without any issues. Restarts of our services now no longer produce duplicates that we got due to failed acknowledge requests during service shutdowns. |
Oh, I've also added 4 test cases for Would be great if this would land in the next release! |
src/subscription.js
Outdated
}); | ||
}); | ||
} | ||
} else { |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
src/subscription.js
Outdated
}); | ||
}); | ||
} | ||
} else { |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
@ctavan looks like our lint task is failing - if you would be so kind as to run |
The acknowledge and modifyAckDeadline API requests seem to have a size limitation: If you send to many ackIds with one request you may get the following error: ``` Request payload size exceeds the limit: 524288 bytes. ``` In order to prevent this error, make sure to always send those requests in chunks that are small enough. Fixes #62.
@callmehiphop whoops, I should have checked linting, sorry for the additional loop. Should be better now. |
@ctavan no worries! I went ahead and ran all the system tests locally and they're all green, so I think we're good to go. Thanks again for the contribution! |
The acknowledge and modifyAckDeadline API requests seem to have a size
limitation: If you send to many ackIds with one request you may get the
following error:
In order to prevent this error, make sure to always send those requests
in chunks that are small enough.
Fixes #62.