[PIP-124] Create init subscription before sending message to DLQ #13355

RobertIndie · 2021-12-16T08:14:57Z

Motivation

If we enable the DLQ when consuming messages. For some messages that can't be processed successfully, the messages will be moved to the DLQ, but if we do not specify the data retention for the namespace or create a subscription for the DLQ to retain the data, the data of the DLQ will be removed automatically. Therefore, we need to create the initial subscription before sending messages to the DLQ.

Modifications

When the deadLetterProducer is initialized, the consumer will set the initial subscription of the deadLetterProducer according to the DeadLetterPolicy.

When the broker creates a producer(after receiving the CommandProducer), if it finds that the topic does not exist, it will not only create the topic(if the topic automatically creation is enabled) but also create the initial subscription(if the initial subscription name is set). When creating an initial subscription, the user needs to have the canConsume permission and the allowAutoSubscriptionCreation needs to be enabled.

Verifying this change

This change is already covered by existing tests, such as
testPermissionForProducerCreateInitialSubscription
testDeadLetterTopicWithInitialSubscription

testDeadLetterTopicWithInitialSubscriptionAndMultiConsumers* and so on.

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

Dependencies (does it add or upgrade a dependency): (no)
The public API: (yes)
The schema: (no)
The default values of configurations: (no)
The wire protocol: (no)
The rest endpoints: (no)
The admin cli options: (no)
Anything that affects deployment: (no)

Documentation

Check the box below and label this PR (if you have committer privilege).

Need to update docs?

doc-required

Motivation If we enable the DLQ when consuming messages. For some messages that can't be processed successfully, the messages will be moved to the DLQ, but if we do not specify the data retention for the namespace or create a subscription for the DLQ to retain the data, the data of the DLQ topic will be removed automatically. Therefore, we need to create the initial subscription before sending messages to the DLQ. Modification * Add `initSubscriptionName` and `initSubscriptionType` to the `DeadLetterPolicy` * Create the initial subscription before creating the deadLetterProducer in ConsumerImpl. Signed-off-by: Zike Yang <[email protected]>

pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerImpl.java

pulsar-client-api/src/main/java/org/apache/pulsar/client/api/DeadLetterPolicy.java

pulsar-broker/src/test/java/org/apache/pulsar/client/api/DeadLetterTopicTest.java

pulsar-client-api/src/main/java/org/apache/pulsar/client/api/DeadLetterPolicy.java

RobertIndie · 2021-12-20T09:58:35Z

@merlimat Thanks for your review. I have created a proposal for this fix: #13408. PTAL

Signed-off-by: Zike Yang <[email protected]>

Motivation This PR is the doc for apache#13355 Modification * Add doc for init sub config in DLQ policy and producer Signed-off-by: Zike Yang <[email protected]>

RobertIndie · 2022-01-28T02:53:02Z

/pulsarbot run-failure-checks

pulsar-client-api/src/main/java/org/apache/pulsar/client/api/ProducerBuilder.java

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java

pulsar-broker/src/test/java/org/apache/pulsar/client/api/AuthorizationProducerConsumerTest.java

Signed-off-by: Zike Yang <[email protected]>

…xception Signed-off-by: Zike Yang <[email protected]>

Signed-off-by: Zike Yang <[email protected]>

hangc0276 · 2022-01-30T07:53:06Z

/pulsarbot run-failure-checks

RobertIndie · 2022-02-08T03:25:24Z

/pulsarbot run-failure-checks

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java

Signed-off-by: Zike Yang <[email protected]>

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java

Signed-off-by: Zike Yang <[email protected]>

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java

Signed-off-by: Zike Yang <[email protected]>

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java

Technoboy-

LGTM

Signed-off-by: Zike Yang <[email protected]>

RobertIndie · 2022-02-13T15:03:05Z

/pulsarbot run-failure-checks

Motivation In apache#13355, we added support for creating initial subscription when creating the producer. But the broker didn't check if the subscription can be created automatically. The initial subscription will be created even if the `allowAutoSubscriptionCreation` is disabled. Modification * Check `allowAutoSubscriptionCreation` when creating the initial subscription. Signed-off-by: Zike Yang <[email protected]>

…#14458) Master Issue: #13408 ### Motivation In #13355, we added support for creating initial subscription when creating the producer. But the broker didn't check if the subscription can be created automatically. The initial subscription will be created even if the `allowAutoSubscriptionCreation` is disabled.

…#14458) Master Issue: #13408 ### Motivation In #13355, we added support for creating initial subscription when creating the producer. But the broker didn't check if the subscription can be created automatically. The initial subscription will be created even if the `allowAutoSubscriptionCreation` is disabled. (cherry picked from commit db25438)

…che#13355) * fix(Client): Create init subscription while creating DLQ producer Motivation If we enable the DLQ when consuming messages. For some messages that can't be processed successfully, the messages will be moved to the DLQ, but if we do not specify the data retention for the namespace or create a subscription for the DLQ to retain the data, the data of the DLQ topic will be removed automatically. Therefore, we need to create the initial subscription before sending messages to the DLQ. Modification * Add `initSubscriptionName` and `initSubscriptionType` to the `DeadLetterPolicy` * Create the initial subscription before creating the deadLetterProducer in ConsumerImpl. Signed-off-by: Zike Yang <[email protected]>

…apache#14458) Master Issue: apache#13408 ### Motivation In apache#13355, we added support for creating initial subscription when creating the producer. But the broker didn't check if the subscription can be created automatically. The initial subscription will be created even if the `allowAutoSubscriptionCreation` is disabled.

lhotari · 2022-09-06T16:46:20Z

What was the purpose of commit 0f202b5 "Make initialSubscriptionName internal"?

RobertIndie · 2022-09-13T14:05:08Z

What was the purpose of commit 0f202b5 "Make initialSubscriptionName internal"?

Exposing the initialSubscriptionName property to the producer is a bit weird. From the user's perspective, the producer should not care about the subscription. So I changed it to internal.

github-actions bot assigned RobertIndie Dec 16, 2021

github-actions bot added the doc-required Your PR changes impact docs and you will update later. label Dec 16, 2021

merlimat reviewed Dec 16, 2021

View reviewed changes

merlimat added this to the 2.10.0 milestone Dec 16, 2021

merlimat reviewed Dec 16, 2021

View reviewed changes

pulsar-client-api/src/main/java/org/apache/pulsar/client/api/DeadLetterPolicy.java Outdated Show resolved Hide resolved

RobertIndie mentioned this pull request Dec 20, 2021

PIP-124: Create init subscription before sending message to DLQ #13408

Closed

sijie mentioned this pull request Dec 20, 2021

ISSUE-13408: PIP-124: Create init subscription before sending message to DLQ streamnative/pulsar-archived#3452

Closed

RobertIndie added 4 commits January 26, 2022 16:14

Merge branch 'master' into fix-dlq

98d647c

Allow creating initial subscription when creating the producer.

a5b4686

Signed-off-by: Zike Yang <[email protected]>

Add more tests

d6afc6c

Signed-off-by: Zike Yang <[email protected]>

Add multi consumers test case

945dc6c

Signed-off-by: Zike Yang <[email protected]>

RobertIndie changed the title ~~[WIP][Client] Create init subscription while creating DLQ producer~~ [PIP-124] Create init subscription before sending message to DLQ Jan 27, 2022

RobertIndie added 2 commits January 27, 2022 18:36

Fix code style.

22709c6

Signed-off-by: Zike Yang <[email protected]>

fix codestyle.

21dd95b

Signed-off-by: Zike Yang <[email protected]>

RobertIndie mentioned this pull request Jan 28, 2022

[Doc] Add doc for init sub config in DLQ policy and producer #13995

Merged

1 task

codelipenghui reviewed Jan 28, 2022

View reviewed changes

RobertIndie added 4 commits January 29, 2022 15:13

Make initialSubscriptionName internal.

0f202b5

Signed-off-by: Zike Yang <[email protected]>

Add ProducerInitSubAuthzException

8691660

Signed-off-by: Zike Yang <[email protected]>

Rename ProducerInitSubAuthzException to ProducerInitSubAuthorizationE…

9933c7f

…xception Signed-off-by: Zike Yang <[email protected]>

Fix test.

305b5e3

Signed-off-by: Zike Yang <[email protected]>

Anonymitaet added doc-complete Your PR changes impact docs and the related docs have been already added. and removed doc-required Your PR changes impact docs and you will update later. labels Feb 7, 2022

Merge branch 'master' into fix-dlq

f71f32e

codelipenghui reviewed Feb 8, 2022

View reviewed changes

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java Show resolved Hide resolved

Apply comments.

4e348aa

Signed-off-by: Zike Yang <[email protected]>

codelipenghui reviewed Feb 10, 2022

View reviewed changes

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java Outdated Show resolved Hide resolved

Use thenCombine to check authz.

123c04a

Signed-off-by: Zike Yang <[email protected]>

codelipenghui approved these changes Feb 10, 2022

View reviewed changes

mattisonchao approved these changes Feb 10, 2022

View reviewed changes

Technoboy- reviewed Feb 10, 2022

View reviewed changes

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java Outdated Show resolved Hide resolved

Apply comments.

7ab9260

Signed-off-by: Zike Yang <[email protected]>

Technoboy- reviewed Feb 13, 2022

View reviewed changes

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java Outdated Show resolved Hide resolved

Technoboy- approved these changes Feb 13, 2022

View reviewed changes

Apply comments.

58b8beb

Signed-off-by: Zike Yang <[email protected]>

codelipenghui merged commit 20f1ba3 into apache:master Feb 14, 2022

RobertIndie mentioned this pull request Feb 24, 2022

[Broker] Check allowAutoSubscriptionCreation when creating init sub #14458

Merged

1 task

shibd mentioned this pull request Dec 6, 2022

[feat] Support Dead Letter Topic. apache/pulsar-client-cpp#139

Merged

4 tasks

[PIP-124] Create init subscription before sending message to DLQ #13355

[PIP-124] Create init subscription before sending message to DLQ #13355

Uh oh!

Conversation

RobertIndie commented Dec 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Modifications

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RobertIndie commented Dec 20, 2021

Uh oh!

RobertIndie commented Jan 28, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hangc0276 commented Jan 30, 2022

Uh oh!

RobertIndie commented Feb 8, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Technoboy- left a comment

Choose a reason for hiding this comment

Uh oh!

RobertIndie commented Feb 13, 2022

Uh oh!

lhotari commented Sep 6, 2022

Uh oh!

RobertIndie commented Sep 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

RobertIndie commented Dec 16, 2021 •

edited

Loading

RobertIndie commented Sep 13, 2022 •

edited

Loading