-
Notifications
You must be signed in to change notification settings - Fork 595
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
Experiencing Extreme (10s+) Latencies with Pub/Sub #1974
Comments
@tmatsuo can you confirm we should be firing multiple pull requests when a user registers a message event (
This is the current behavior, with the exception of the 10 millisecond timeout. This can be changed: var subscription = topic.subscription('sub-name', { timeout: 0 }) |
Developer on the Cloud Pub/Sub team here. I can confirm that multiple outstanding pull requests is the correct behavior. The number of concurrent requests would have to grow fairly large before it would be an issue on the server side. 5-10 simultaneous pull requests would be reasonable. |
This is still a significant problem for us (beyond performance). Is there an ETA on a fix? I'm not sure how GCP bugs are prioritized and do realize the API is Alpha quality. |
On it! |
@MarkHerhold could you give this a try: $ npm install --save @google-cloud/[email protected] |
@stephenplusplus Yeah, it breaks after a few sequential publish/subscribes. This isn't the most elegant test script, but it reproduces the problem for me, usually sometime after the 10th iteration. 🤔 const pubsub = require('@google-cloud/pubsub');
const client = pubsub(/* secret here */);
async function go() {
const topic = (await client.createTopic('sometopic' + Math.floor(Math.random() * 2000)))[0];
const subscription = (await topic.subscribe({
ackDeadlineSeconds: 10,
autoAck: false
}))[0];
for (let i = 0; i < 20; i++) {
const start = Date.now();
await new Promise(function(resolve, reject) {
function onMessage(message) {
if (message.data === 'test') {
subscription.removeListener('message', onMessage);
message.ack().then(resolve).catch(reject);
}
}
subscription.on('message', onMessage);
topic.publish('test');
setTimeout(function() {
subscription.removeListener('message', onMessage);
reject('Pub/Sub timed out after ' + 10000 + ' ms');
}, 10000);
}).then(function() {
console.log(`Pub/Sub resolved in ${Date.now() - start} ms`);
}).catch(function(err) {
console.error(err);
});
}
}
try {
go();
} catch (err) {
console.error(err);
} |
@MarkHerhold I think that maybe your issue is similar to mine. The message counts as pulled, but can't be acked for some reason, then you have to wait until the |
Can I have a rough ballpark estimate of when this can be looked at/fixed? An estimate would really help me plan my own features. Thank you! |
Thanks for your patience on this. The Pub/Sub team reviewed our API and had a lot of feedback for us, which could see us re-designing big parts of how our API works. These changes will probably roll out over the next few weeks (no guarantee), but would likely address this issue at the same time. Given the big changes ahead, I'm hesitant to perfect a solution (i.e., what #2006 was meant to evolve into), when the feedback from the API team could squash it. If this isn't explicitly covered from their feedback, we will make sure it's addressed after the bigger changes we make to the API. |
@stephenplusplus Ok, thanks for the insight! |
OK, that's really good to know. We've been having a lot of issues with PubSub, especially when writing tests for it. I'm looking forward to major improvements to the NodeJS lib for it :) |
Thanks to both of you for your help along the way! |
@stephenplusplus Hey, was there a change last night? My latency for first request went from 70s+ to 2s+ Edit: |
No change last night. I would love to know what your new node modules are. |
Hey, any ETA on updates to pubsub here? |
I am pulling together a design proposal, and will be assigning it sometime in the next couple weeks. |
@lukesneeringer @stephenplusplus |
A redesign is currently in progress. Expect a PR within the coming week or so. |
I'm going to close as the new PR is about to be sent. The Pub/Sub team was behind the entire redesign, so it will be using the best practices, following their recommendations. |
The PR has been sent: #2380. Thanks for your patience, everyone. |
Awesome that the PR is here! But it seems premature to close this as the PR still has a number of TODOs and a 'do not merge' tag. Thanks for your hard work on this issue and it's great news that the Pub/Sub team was involved! |
I agree with @Splaktar . It is unfair to customers when issues get closed before they are actually resolved. Please consider keeping this open until the PR is merged for tracking and visibility purposes. |
Thanks for the feedback! I will re-open, with a quick explanation. Our APIs are shifting towards using a generated layer, meaning it would be the fault of that, or a child layer if this problem persisted. Keeping this open makes a lot of sense, and closing it wouldn't guarantee a resolution. Thanks for keeping us doing the right things-- always feel free to speak your mind if we head down the wrong road again. |
@MarkHerhold we were seeing the same exact behavior in our pub/sub POC. I can confirm that so far #2380 has resolved the random 10s+ delay. |
The new release is out! Please upgrade to |
@stephenplusplus If i was using subscription.pull({ returnImmediately: true }) earlier, is there some guidance on how i should do it instead with the api re-design. Thanks |
I described my situation in this GCP Pub/Sub forum post:
https://groups.google.com/forum/#!topic/cloud-pubsub-discuss/xhDvw63IVI8
Kamal Aboul-Hosn said to create an issue on this repo and commented that it may be a perf issue. Here's what he said:
Environment details
Steps to reproduce
@google-cloud/pubsub
Image from monitoring tool showing frequency of timeouts over the past 24 hours:
The text was updated successfully, but these errors were encountered: