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

deprecate pubsub PullRequest returnImmediately flag #309

Closed
lukasGemela opened this issue Feb 15, 2021 · 10 comments
Closed

deprecate pubsub PullRequest returnImmediately flag #309

lukasGemela opened this issue Feb 15, 2021 · 10 comments
Assignees
Labels

Comments

@lukasGemela
Copy link
Contributor

Is your feature request related to a problem? Please describe.
whilst investigating problems with delayed pubsub messages we found out that our problems were caused by returnImmediately flag set to true. According to documentation this flag is actually deprecated server-side and they heavily discourage from using it now:
https://cloud.google.com/pubsub/docs/reference/rpc/google.pubsub.v1#pullrequest

Describe the solution you'd like
Either remove it or deprecate it using annotation.

Describe alternatives you've considered
No alternatives.

Additional context
We could also do the PR but I open this report to run decision whether keep the flag in deprecated state or remove it completely.

@lukasGemela lukasGemela changed the title reprecate pubsub PullRequest returnImmediately flag deprecate pubsub PullRequest returnImmediately flag Feb 15, 2021
@dzou
Copy link
Contributor

dzou commented Feb 16, 2021

Thanks for the report. We had discussions of deprecating this in the past, though had trouble coordinating this change with the Pub/Sub team. Will follow-up and ask what is our long-term plan for this. @meltsufin @elefeint

@meltsufin
Copy link
Member

Related to: spring-attic/spring-cloud-gcp#2247.
If I recall correctly, we decided to keep it last time we looked at it because there was no directly replacement for it.

@lukasGemela
Copy link
Contributor Author

I think my biggest issue is that setting returnImmediately to true has some serious side effect for us (message delivery delayed for hours), the flag itself is deprecated and there is no mentioning in documentation or javaDoc. Spring template documentation should at least discourage people from using this flag

@meltsufin
Copy link
Member

I think my biggest issue is that setting returnImmediately to true has some serious side effect for us (message delivery delayed for hours)

This sounds like a bug potentially. Can you elaborate how the use of this flag delays message delivery?

@lukasGemela
Copy link
Contributor Author

lukasGemela commented Mar 1, 2021

I think my biggest issue is that setting returnImmediately to true has some serious side effect for us (message delivery delayed for hours)

This sounds like a bug potentially. Can you elaborate how the use of this flag delays message delivery?

sure, please read through this ticket
https://issuetracker.google.com/issues/180098468

Also google doc states

Warning: setting this field to true is discouraged because it adversely impacts the performance of subscriptions.pull operations. We recommend that users do not set this field.

@meltsufin
Copy link
Member

@lukasGemela Thanks for the reference to the ticket. We'll work internally to figure out how to proceed and update this issue soon.

@meltsufin meltsufin self-assigned this Mar 1, 2021
@meltsufin
Copy link
Member

@lukasGemela The word from the Pub/Sub team is that the way returnImmediately=true works at the moment is that it waits up to 1 second before returning the pull call, and it's only kind of deprecated because there are still users who depend on its behavior and there is no direct replacement yet.
It's also not recommended because it will generally decrease throughput and latency.

An alternative is to use the streaming subscriber. Have you explored that?

@meltsufin
Copy link
Member

I also got more details on why returnImmediately=true may cause very long delays in message delivery. Since it only waits for up to 1 second, it could happen that it is just not enough time to retrieve any messages internally from the Pub/Sub servers. This can happen repeatedly, causing very long delays. Hence, it's not recommended.

@lukasGemela
Copy link
Contributor Author

@lukasGemela The word from the Pub/Sub team is that the way returnImmediately=true works at the moment is that it waits up to 1 second before returning the pull call, and it's only kind of deprecated because there are still users who depend on its behavior and there is no direct replacement yet.
It's also not recommended because it will generally decrease throughput and latency.

An alternative is to use the streaming subscriber. Have you explored that?

yes, I did, but that does not really fit our use case. But we just set returnImmediately flag to false and it works fine now, as stated under that google issue. I would at least add something into clients javadoc/documentation that setting this flag to true is not recommended and why 🤷‍♂️

@meltsufin
Copy link
Member

Yes, we'll do that. Thanks for bringing up this issue so that we can document it better.

meltsufin added a commit that referenced this issue Mar 5, 2021
Updates javadocs and reference documentation with a warning.
However, we decided not to deprecate the field for now.

Addresses: #309.
kateryna216 added a commit to kateryna216/spring-cloud-gcp that referenced this issue Oct 20, 2022
…m#354)

Updates javadocs and reference documentation with a warning.
However, we decided not to deprecate the field for now.

Fixes: GoogleCloudPlatform#309.
prash-mi pushed a commit that referenced this issue Jun 20, 2023
Sonar rule: "JUnit5 test classes and methods should have default package visibility"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants