I think we can create a consumer with the DLQ init subscription and then close the consumer? This will make the implementation easier.
Penghui On Wed, Dec 22, 2021 at 4:49 PM Zike Yang <zky...@streamnative.io.invalid> wrote: > > We can avoid confusion by only attempting to create the subscription > > when `initSubscriptionName` is not `null`. As a result, this PIP > > wouldn't change the behavior for current implementations. > I think there is an issue with the current behavior. When the init > subscription > is not created before creating deadLetterProducer, it may result in the > loss of messages. > And this is not the expected behavior. So the purpose of this PIP is to fix > this issue. > IMO, we need to create that subscription by default to fix the current > behavior. > > > +1 - I think we should expand the binary protocol to add a > > `CommandSubscription` or possibly `CommandCreateSubscription` > > (`CommandSubscribe` is already taken). Without a new command, we will > > introduce race conditions related to consumer creation and will > > definitely generate unnecessary errors. > +1. `CommandCreateSubscription` seems more reasonable. > > > Regarding default authorization, this new command would require the > > same level of permission for the client since the act of creating a > > consumer and the act of creating a subscription both require the same > > `AuthAction` permission: Consume. > +1 > > Thanks, > Zike Yang > > On Wed, Dec 22, 2021 at 2:40 AM Michael Marshall <mmarsh...@apache.org> > wrote: > > > > > IMO, we won't create the init subscription if > `allowAutoSubscriptionCreation` > > > is false. Otherwise, it will confuse the user. Users can explicitly > create that > > > subscription to handle this case. > > > > That is a good point, but I don't think we should attempt to create > > the subscription implicitly via consumer creation because that will > > lead to additional failure, which adds unnecessary load on the broker. > > > > We can avoid confusion by only attempting to create the subscription > > when `initSubscriptionName` is not `null`. As a result, this PIP > > wouldn't change the behavior for current implementations. > > > > > Do you mean to create a private method called `createSubscription` in > > > the non-admin client? But we don't have a protocol command for > > > creating subscription > > > > Yes, that was what I meant, and you're right, we don't have a protocol > > command for this case. > > > > > Or the other way is to create a new command `CommandSubscription` in > > > the protocol to handle this. > > > > +1 - I think we should expand the binary protocol to add a > > `CommandSubscription` or possibly `CommandCreateSubscription` > > (`CommandSubscribe` is already taken). Without a new command, we will > > introduce race conditions related to consumer creation and will > > definitely generate unnecessary errors. > > > > Regarding default authorization, this new command would require the > > same level of permission for the client since the act of creating a > > consumer and the act of creating a subscription both require the same > > `AuthAction` permission: Consume. > > > > Thanks, > > Michael > > > > On Tue, Dec 21, 2021 at 4:57 AM Zike Yang > > > > <zky...@streamnative.io.invalid> wrote: > > > > > > Thanks for your review Michael. > > > > > > On Tue, Dec 21, 2021 at 5:48 AM Michael Marshall <mmarsh...@apache.org > > > wrote: > > > > > > > > Thanks for creating this PIP Zike Yang. This sounds like an important > > > > improvement. I have a couple thoughts. > > > > > > > > 1. Does the `retryLetterTopic` have the same problem with immediate > > > > message expiration? Based on a quick glance, I don't see anything > > > > creating a subscription for the retry topic. > > > > > > > Yes. I think we also need to handle the retryLetterTopic. I will update > the PIP. > > > > > > > 2. Your prototype exposes a lacking in our existing client > > > > implementation: the non-admin client cannot directly create > > > > subscriptions. IIUC, this implementation does not work if > > > > `allowAutoSubscriptionCreation` is false. > > > IMO, we won't create the init subscription if > `allowAutoSubscriptionCreation` > > > is false. Otherwise, it will confuse the user. Users can explicitly > create that > > > subscription to handle this case. > > > > > > > Further, it is inefficient > > > > to create and immediately close a consumer just to create a > > > > subscription. Perhaps we should just update the non-admin client so > > > > that it can create subscriptions? It could be private so that we > don't > > > > create confusion with the non-admin and the admin clients. > > > Do you mean to create a private method called `createSubscription` in > > > the non-admin client? But we don't have a protocol command for > > > creating subscription. > > > So in that method, we can send the `CommandSubscribe` and > `CommandUnsubscribe` > > > to the broker to simulate the creation and closure of a consumer. But > > > I think it is not > > > straightforward to create a subscription using two commands. > > > Or the other way is to create a new command `CommandSubscription` in > > > the protocol > > > to handle this. But I wonder if there are more use cases for this > > > command that would > > > make it worthwhile to use this solution. > > > What do you think about these two ways? > > > > > > > > > > > Thanks, > > > > Michael > > > > > > > > > > > > On Mon, Dec 20, 2021 at 4:18 AM Zike Yang > > > > <zky...@streamnative.io.invalid> wrote: > > > > > > > > > > https://github.com/apache/pulsar/issues/13408 > > > > > > > > > > Pasted below for quoting convenience. > > > > > > > > > > —— > > > > > > > > > > > > > > > ## 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. > > > > > > > > > > ## Goal > > > > > > > > > > Users can set the initial subscription name in the > DeadLetterPolicy. The consumer will create the initial subscription before > sending messages to the DLQ. At this point, subsequent messages produced to > the DLQ are not automatically deleted unexpectedly. > > > > > > > > > > This PIP needs to be compatible with the previous behavior. The > initial subscription name in the DeadLetterPolicy is optional. Default > value is the subscription name of the consumer. > > > > > > > > > > ## API Changes > > > > > > > > > > Add `initSubscriptionName` to the `DeadLetterPolicy` > > > > > > > > > > ```java > > > > > /** > > > > > * Name of the initial subscription name of the dead letter topic. > > > > > * The default value is the subscription name of the consumer. > > > > > */ > > > > > private String initSubscriptionName; > > > > > ``` > > > > > > > > > > ## Implementation > > > > > > > > > > Before the deadLetterProducer is initialized, the consumer first > tries to create a deadLetterConsumer using the initial subscription name in > the DeadLetterPolicy. When this subscription already exists, the > ConsumerBusy exception will occur. In this case, we can ignore that > exception and create the deadLetterProducer. > > > > > > > > > > Prototype implementation PR: > https://github.com/apache/pulsar/pull/13355 > > > > > > > > > > > > > > > > > > > > Thanks, > > > > > Zike Yang > > > > > > > > > > > > > > > > > Best regards, > > > Zike Yang >
