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.

Sign up for GitHub

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

Jump to bottom

feat: Add dead lettering max delivery attempts argument #236

Merged
merged 7 commits into from Nov 16, 2020
Merged

feat: Add dead lettering max delivery attempts argument #236

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d8e9047
Add max_delivery_attempts input to subsciber.py
danavaziri-ga Nov 10, 2020
911300e
Make max_delivery_attempts argument optional
danavaziri-ga Nov 11, 2020
28569e5
Add max_delivery_attempts parameter to create and update subscription…
danavaziri-ga Nov 11, 2020
5fb0431
Merge branch 'master' into patch-1
danavaziri-ga Nov 13, 2020
7ad3b6a
Add constants and defaults for max_delivery_attempts argument
danavaziri-ga Nov 13, 2020
332bcce
Fix comments related to added max_delivery_attempts parameter
danavaziri-ga Nov 13, 2020
52db582
Fix typo in max_delivery_attempts comments
danavaziri-ga Nov 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
41 changes: 30 additions & 11 deletions samples/snippets/subscriber.py
View file
Open in desktop
Expand Up @@ -90,7 +90,8 @@ def create_subscription(project_id, topic_id, subscription_id):


def create_subscription_with_dead_letter_topic(
project_id, topic_id, subscription_id, dead_letter_topic_id
project_id, topic_id, subscription_id, dead_letter_topic_id,
max_delivery_attempts
):
"""Create a subscription with dead letter policy."""
# [START pubsub_dead_letter_create_subscription]
Expand All @@ -108,6 +109,9 @@ def create_subscription_with_dead_letter_topic(
# TODO(developer): This is an existing dead letter topic that the subscription
# with dead letter policy will forward dead letter messages to.
# dead_letter_topic_id = "your-dead-letter-topic-id"
# TODO(developer): This is an the maximum number of delivery attempts allowed
# before a message gets dead lettered.
# max_delivery_attempts = "your-max-delivery-attempts"
danavaziri-ga marked this conversation as resolved.
Show resolved Hide resolved

publisher = pubsub_v1.PublisherClient()
subscriber = pubsub_v1.SubscriberClient()
Expand All @@ -117,7 +121,8 @@ def create_subscription_with_dead_letter_topic(
dead_letter_topic_path = publisher.topic_path(project_id, dead_letter_topic_id)

dead_letter_policy = DeadLetterPolicy(
dead_letter_topic=dead_letter_topic_path, max_delivery_attempts=10
dead_letter_topic=dead_letter_topic_path,
max_delivery_attempts=max_delivery_attempts
)

with subscriber:
Expand Down Expand Up @@ -259,7 +264,8 @@ def update_push_subscription(project_id, topic_id, subscription_id, endpoint):


def update_subscription_with_dead_letter_policy(
project_id, topic_id, subscription_id, dead_letter_topic_id
project_id, topic_id, subscription_id, dead_letter_topic_id,
max_delivery_attempts
):
"""Update a subscription's dead letter policy."""
# [START pubsub_dead_letter_update_subscription]
Expand All @@ -276,6 +282,9 @@ def update_subscription_with_dead_letter_policy(
# TODO(developer): This is an existing dead letter topic that the subscription
# with dead letter policy will forward dead letter messages to.
# dead_letter_topic_id = "your-dead-letter-topic-id"
# TODO(developer): This is an the maximum number of delivery attempts allowed
# before a message gets dead lettered.
# max_delivery_attempts = "your-max-delivery-attempts"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.


publisher = pubsub_v1.PublisherClient()
subscriber = pubsub_v1.SubscriberClient()
Expand All @@ -290,11 +299,12 @@ def update_subscription_with_dead_letter_policy(
print(f"Before the update: {subscription_before_update}.")

# Indicates which fields in the provided subscription to update.
update_mask = FieldMask(paths=["dead_letter_policy.max_delivery_attempts"])
update_mask = FieldMask(paths=["dead_letter_policy"])

# Construct a dead letter policy you expect to have after the update.
dead_letter_policy = DeadLetterPolicy(
dead_letter_topic=dead_letter_topic_path, max_delivery_attempts=20
dead_letter_topic=dead_letter_topic_path,
max_delivery_attempts=max_delivery_attempts
)

# Construct the subscription with the dead letter policy you expect to have
Expand Down Expand Up @@ -339,12 +349,7 @@ def remove_dead_letter_policy(project_id, topic_id, subscription_id):
print(f"Before removing the policy: {subscription_before_update}.")

# Indicates which fields in the provided subscription to update.
update_mask = FieldMask(
paths=[
"dead_letter_policy.dead_letter_topic",
"dead_letter_policy.max_delivery_attempts",
]
)
update_mask = FieldMask(paths=["dead_letter_policy"])

# Construct the subscription (without any dead letter policy) that you
# expect to have after the update.
Expand Down Expand Up @@ -676,6 +681,12 @@ def callback(message):
create_with_dead_letter_policy_parser.add_argument("topic_id")
create_with_dead_letter_policy_parser.add_argument("subscription_id")
create_with_dead_letter_policy_parser.add_argument("dead_letter_topic_id")
create_with_dead_letter_policy_parser.add_argument(
"max_delivery_attempts",
type=int,
nargs="?",
default=5
)

create_push_parser = subparsers.add_parser(
"create-push", help=create_push_subscription.__doc__
Expand Down Expand Up @@ -707,6 +718,12 @@ def callback(message):
update_dead_letter_policy_parser.add_argument("topic_id")
update_dead_letter_policy_parser.add_argument("subscription_id")
update_dead_letter_policy_parser.add_argument("dead_letter_topic_id")
update_dead_letter_policy_parser.add_argument(
"max_delivery_attempts",
type=int,
nargs="?",
default=5
)

remove_dead_letter_policy_parser = subparsers.add_parser(
"remove-dead-letter-policy", help=remove_dead_letter_policy.__doc__
Expand Down Expand Up @@ -777,6 +794,7 @@ def callback(message):
args.topic_id,
args.subscription_id,
args.dead_letter_topic_id,
args.max_delivery_attempts,
)
elif args.command == "create-push":
create_push_subscription(
Expand All @@ -798,6 +816,7 @@ def callback(message):
args.topic_id,
args.subscription_id,
args.dead_letter_topic_id,
args.max_delivery_attempts,
)
elif args.command == "remove-dead-letter-policy":
remove_dead_letter_policy(args.project_id, args.topic_id, args.subscription_id)
Expand Down
4 changes: 2 additions & 2 deletions samples/snippets/subscriber_test.py
View file
Open in desktop
Expand Up @@ -208,7 +208,7 @@ def test_create_subscription_with_dead_letter_policy(
pass

subscriber.create_subscription_with_dead_letter_topic(
PROJECT_ID, TOPIC, SUBSCRIPTION_DLQ, DEAD_LETTER_TOPIC
PROJECT_ID, TOPIC, SUBSCRIPTION_DLQ, DEAD_LETTER_TOPIC, 10
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question for this line and line 222

  • What are the values 10 and 20? Why did we choose them to be 10 and 20 and not whatever the default is?
  • If we do need to pass them and not use the default, please use a constant (or two if you want to have the separate values 10&20) like the other arguments passed in the tests - it makes for easier maintenance

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I addressed this in a seperate commit 7ad3b6a.

I added two constants one for the default value and one for the updated one. We could call create subscription with the default value, which is what my new commit does. However, the update subscription call should be called with a new value because we are testing to see if max_delivery_attempts changes after the call.

)

out, _ = capsys.readouterr()
Expand All @@ -219,7 +219,7 @@ def test_create_subscription_with_dead_letter_policy(

def test_update_dead_letter_policy(subscription_dlq, dead_letter_topic, capsys):
_ = subscriber.update_subscription_with_dead_letter_policy(
PROJECT_ID, TOPIC, SUBSCRIPTION_DLQ, DEAD_LETTER_TOPIC
PROJECT_ID, TOPIC, SUBSCRIPTION_DLQ, DEAD_LETTER_TOPIC, 20
)

out, _ = capsys.readouterr()
Expand Down