fix: add missing retry to publisher.publish in gcpubsub _put#2465
fix: add missing retry to publisher.publish in gcpubsub _put#2465poolik wants to merge 6 commits intocelery:mainfrom
Conversation
The publish call in _put was missing a retry parameter, unlike _put_fanout which already had one. This could cause transient failures to propagate immediately instead of being retried. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes a missing retry configuration in the GCP Pub/Sub transport's _put method. The publisher.publish call in _put was missing the retry parameter that was already present in the analogous _put_fanout method and all other GCP Pub/Sub API calls throughout the file. This inconsistency could cause transient failures in direct queue publishing to propagate immediately instead of being retried.
Changes:
- Added
retry=Retry(deadline=self.retry_timeout_seconds)parameter to thepublisher.publishcall in the_putmethod
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2465 +/- ##
=======================================
Coverage 81.13% 81.13%
=======================================
Files 77 77
Lines 9775 9775
Branches 1104 1104
=======================================
Hits 7931 7931
Misses 1636 1636
Partials 208 208 ☔ View full report in Codecov by Sentry. |
auvipy
left a comment
auvipy
left a comment
There was a problem hiding this comment.
would this be better adding extra Unit test for this?
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
for more information, see https://pre-commit.ci
|
@auvipy I've added unit test as well :) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| call_kwargs = channel.publisher.publish.call_args[1] | ||
| assert isinstance(call_kwargs['retry'], Retry) | ||
| assert call_kwargs['retry']._timeout == 60 |
There was a problem hiding this comment.
This assertion relies on the private Retry._timeout attribute, which is version-dependent and can make the test brittle. Consider validating the Retry configuration without reaching into private internals (e.g., by patching kombu.transport.gcpubsub.Retry and asserting it was called with the expected deadline).
There was a problem hiding this comment.
can you please cross check this suggestion?
| call_kwargs = channel.publisher.publish.call_args[1] | ||
| assert isinstance(call_kwargs['retry'], Retry) | ||
| assert call_kwargs['retry']._timeout == 300 |
There was a problem hiding this comment.
The test is asserting against Retry's private attribute ._timeout. This is not part of the public API and can change across google-api-core versions (and may not reflect the value passed via deadline=). Prefer asserting via a stable interface (e.g., patching kombu.transport.gcpubsub.Retry and checking it was constructed with deadline=..., or asserting on a public property if one exists in the supported versions).
auvipy
left a comment
auvipy
left a comment
There was a problem hiding this comment.
and please fix the merge conflict
The publish call in _put was missing a retry parameter, unlike _put_fanout which already had one. This could cause transient failures to propagate immediately instead of being retried.