> 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.
I'm not sure I agree with this assertion. The default in Pulsar is that a topic does not retain messages until there is a subscription, and because topic creation does not (currently) imply subscription creation, auto topic creation can lead to message loss. I do agree though that it would be helpful to try to remove the risk posed by auto topic creation. > I think we can create a consumer with the DLQ init subscription and then > close the consumer? This is the current design proposed by this PIP. As I mentioned earlier, I do not think we should take this approach. I think the underlying feature request is to make it possible to auto create a subscription when a topic is auto created. This feature seems similar to `force_topic_creation`, which is already a field on the `CommandSubscribe` command. It is documented here [0]: > // If true, the subscribe operation will cause a topic to be > // created if it does not exist already (and if topic auto-creation > // is allowed by broker. > // If false, the subscribe operation will fail if the topic > // does not exist. > optional bool force_topic_creation = 15 [default = true]; What if we extended the `CommandProducer` command to add a `create_subscription` field? Then, any time a topic is auto created and this field is true, the broker would auto create a subscription. There are some details to work out, but I think this feature would fulfill the needs of this PIP and would also be broadly useful for many client applications that dynamically create topics. Thanks, Michael [0] https://github.com/apache/pulsar/blob/6657a6c2fb23be579e74bac7c7c07a70cabfb4b2/pulsar-common/src/main/proto/PulsarApi.proto#L373-L378 On Wed, Dec 22, 2021 at 5:32 AM PengHui Li <peng...@apache.org> wrote: > > 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 > >
