> 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
