feat: add support for dead letter policies #2504
Conversation
googlebot
added
the
cla: yes
|
cc @pradn |
PubSub/src/Message.php
Outdated
| @@ -70,6 +75,10 @@ class Message | |||
| * | |||
| * @type string $ackId The message ackId. This is only set when messages | |||
| * are pulled from the PubSub service. | |||
| * @type string $deliveryAttempt Delivery attempt counter is 1 + (the | |||
| * sum of number of NACKs and number of ack_deadline exceeds) for | |||
| { | ||
| if ($client instanceof PubSubClientRest) { | ||
| $this->markTestSkipped( | ||
| 'deadLetterPolicy not available in REST transport during experimental period.' |
There was a problem hiding this comment.
I'm unsure when the new feature will be published to the REST definition. We've had the same issue with ordering keys.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Has this been actually resolved?
There was a problem hiding this comment.
The latest version of the discovery document does not include dead letter queues: https://pubsub.googleapis.com/$discovery/rest?version=v1
There was a problem hiding this comment.
Should we notate this in the client?
|
Okay, did an initial review. Just small things. Looks good, overall. |
|
@dwsupplee Can you help review this? Thanks! |
PubSub/src/Message.php
Outdated
| @@ -70,6 +75,10 @@ class Message | |||
| * | |||
| * @type string $ackId The message ackId. This is only set when messages | |||
| * are pulled from the PubSub service. | |||
| * @type string $deliveryAttempt Delivery attempt counter is 1 + (the | |||
| if (isset($subscription['deadLetterPolicy'])) { | ||
| if ($subscription['deadLetterPolicy']['deadLetterTopic'] instanceof Topic) { | ||
| $topic = $subscription['deadLetterPolicy']['deadLetterTopic']; | ||
| $subscription['deadLetterPolicy']['deadLetterTopic'] = $topic->name(); |
There was a problem hiding this comment.
What about maximum delivery attempts field?
There was a problem hiding this comment.
No transformation is required to make the max delivery attempts field compatible with the API.
| { | ||
| if ($client instanceof PubSubClientRest) { | ||
| $this->markTestSkipped( | ||
| 'deadLetterPolicy not available in REST transport during experimental period.' |
There was a problem hiding this comment.
Has this been actually resolved?
| * will fail if the topic does not exist. Users should ensure that | ||
| * there is a subscription attached to this topic since messages | ||
| * published to a topic with no subscriptions are lost. | ||
| * @type int $deadLetterPolicy.maxDeliveryAttempts The maximum number of |
There was a problem hiding this comment.
I don't see maxDeliveryAttempts field being used in any tests. We should write some tests with custom maxDeliveryAttempts and some with default maxDeliveryAttempts (5).
There was a problem hiding this comment.
added system test coverage.
PubSub/tests/Unit/MessageTest.php
Outdated
| @@ -87,6 +88,11 @@ public function testAckId() | |||
| $this->assertEquals(1234, $this->message->ackId()); | |||
| } | |||
|
|
|||
| public function testDeliveryAttempt() | |||
There was a problem hiding this comment.
Should we add a unit test for null delivery attempt?
| { | ||
| if ($client instanceof PubSubClientRest) { | ||
| $this->markTestSkipped( | ||
| 'deadLetterPolicy not available in REST transport during experimental period.' |
There was a problem hiding this comment.
Should we notate this in the client?
I refactored the Grpc.php unit test file, the provider is too difficult to manage.
Closes #2571.