Can we fail the client if they choose NonDurable subscription mode on a
non-persistent topic, instead of writing WARN log messages?
On Mon, Feb 13, 2023 at 4:26 AM guo jiwei <techno...@apache.org> wrote:
> Agree，+1
>
>
> Regards
> Jiwei Guo (Tboy)
>
> On Thu, Feb 9, 2023 at 7:07 PM Jiuming Tao
> <jm...@streamnative.io.invalid> wrote:
> >
> >
> > It makes sense
> >
> > Thanks,
> > Tao Jiuming
> >
> >
> > > 2023年2月9日 15:44，Baodi Shi <ba...@apache.org> 写道：
> > >
> > > Agree, and we can add verification on the client side. When the topic
> is
> > > non-persistent and uses durable to subscribe, print warns logs:
> > > “non-persistent not support durable subscribe will use non-durable to
> > > subscribe.”
> > >
> > >
> > >
> > > Thanks,
> > > Baodi Shi
> > >
> > > 在 2023年2月9日 15:10:12 上，Jiuming Tao <jm...@streamnative.io.invalid> 写道：
> > >
> > >> Agree. But we need to take care of the compatibility. How do we handle
> > >>
> > >> this compatibility?
> > >>
> > >>
> > >> How about set `isDurable = false` when create
> `NonPersistentSubscription`?
> > >>
> > >> In `NonPersistentSubscription`, we can change the constructor from
> > >> ```
> > >> public NonPersistentSubscription(NonPersistentTopic topic, String
> > >> subscriptionName, boolean isDurable,
> > >> Map<String, String> properties) {
> > >> this.topic = topic;
> > >> this.topicName = topic.getName();
> > >> this.subName = subscriptionName;
> > >> this.fullName = MoreObjects.toStringHelper(this).add("topic",
> > >> topicName).add("name", subName).toString();
> > >> IS_FENCED_UPDATER.set(this, FALSE);
> > >> this.lastActive = System.currentTimeMillis();
> > >> this.isDurable = isDurable;
> > >> this.subscriptionProperties = properties != null
> > >> ? Collections.unmodifiableMap(properties) :
> > >> Collections.emptyMap();
> > >> }
> > >>
> > >> ```
> > >>
> > >> to:
> > >>
> > >> ```
> > >> public NonPersistentSubscription(NonPersistentTopic topic, String
> > >> subscriptionName,
> > >> Map<String, String> properties) {
> > >> this.topic = topic;
> > >> this.topicName = topic.getName();
> > >> this.subName = subscriptionName;
> > >> this.fullName = MoreObjects.toStringHelper(this).add("topic",
> > >> topicName).add("name", subName).toString();
> > >> IS_FENCED_UPDATER.set(this, FALSE);
> > >> this.lastActive = System.currentTimeMillis();
> > >> this.isDurable = false;
> > >> this.subscriptionProperties = properties != null
> > >> ? Collections.unmodifiableMap(properties) :
> > >> Collections.emptyMap();
> > >> }
> > >>
> > >> ```
> > >>
> > >> In the broker side, for non-persistent topic, there is no durable
> > >> subscriptions.
> > >> In the client side, users don’t need to change their code.
> > >>
> > >> Thanks,
> > >> Tao Jiuming
> > >>
> > >> 2023年2月9日 15:01，Zike Yang <z...@apache.org> 写道：
> > >>
> > >>
> > >>> It makes sense, for non-persistent topic, it should not create a
> durable
> > >> subscription.
> > >>
> > >>
> > >> Agree. But we need to take care of the compatibility. How do we handle
> > >>
> > >> this compatibility?
> > >>
> > >>
> > >> BR,
> > >>
> > >> Zike Yang
> > >>
> > >>
> > >> On Thu, Feb 9, 2023 at 2:43 PM Jiuming Tao
> > >>
> > >> <jm...@streamnative.io.invalid> wrote:
> > >>
> > >>>
> > >>
> > >>>> We can add validation to inform the user that a durable
> subscription on
> > >> a
> > >>
> > >>>> non-persistent topic is invalid.
> > >>
> > >>>
> > >>
> > >>> It makes sense, for non-persistent topic, it should not create a
> durable
> > >> subscription.
> > >>
> > >>>
> > >>
> > >>> Thanks,
> > >>
> > >>> Tao Jiuming
> > >>
> > >>>
> > >>
> > >>>> 2023年2月8日 23:06，Baodi Shi <ba...@apache.org> 写道：
> > >>
> > >>>>
> > >>
> > >>>>>
> > >>
> > >>>>> But it seems that currently all Durable subscriptions on
> > >>
> > >>>>>
> > >>
> > >>>> non-persistent topics are NonDurable, and I wonder if that confuses
> > >>
> > >>>>> users.
> > >>
> > >>>>>
> > >>
> > >>>>
> > >>
> > >>>> We can add validation to inform the user that a durable
> subscription on
> > >> a
> > >>
> > >>>> non-persistent topic is invalid.
> > >>
> > >>>>
> > >>
> > >>>>
> > >>
> > >>>> Thanks,
> > >>
> > >>>> Baodi Shi
> > >>
> > >>>>
> > >>
> > >>>>
> > >>
> > >>>> 在 2023年2月8日 10:48:32 上，Zike Yang <z...@apache.org> 写道：
> > >>
> > >>>>
> > >>
> > >>>>> I agree to remove all subscriptions when there're no consumers on
> the
> > >>
> > >>>>> non-persistent topic. The configuration
> > >>
> > >>>>> `subscriptionExpirationTimeMinutes ` should not take effect on the
> > >>
> > >>>>> non-persistent topic.
> > >>
> > >>>>>
> > >>
> > >>>>> But it seems that currently all Durable subscriptions on
> > >>
> > >>>>> non-persistent topics are NonDurable, and I wonder if that confuses
> > >>
> > >>>>> users.
> > >>
> > >>>>>
> > >>
> > >>>>>> But for `NonPersistentDispatcherMultipleConsumers`,
> > >>
> > >>>>> `NonPersistentStickyKeyDispatcherMultipleConsumers`, I’m not sure
> that
> > >>
> > >>>>> if it matters when we auto delete the subscriptions which has no
> > >>
> > >>>>> consumers connected.
> > >>
> > >>>>>
> > >>
> > >>>>> It doesn't matter IMO.
> > >>
> > >>>>>
> > >>
> > >>>>> Thanks,
> > >>
> > >>>>> Zike Yang
> > >>
> > >>>>>
> > >>
> > >>>>>
> > >>
> > >>>>>
> > >>
> > >>>>> Zike Yang
> > >>
> > >>>>>
> > >>
> > >>>>> On Tue, Feb 7, 2023 at 9:22 PM Jiuming Tao
> > >>
> > >>>>> <jm...@streamnative.io.invalid> wrote:
> > >>
> > >>>>>
> > >>
> > >>>>>
> > >>
> > >>>>>> I think we are talking about the durable subscription of the
> > >>
> > >>>>> non-persistent
> > >>
> > >>>>>
> > >>
> > >>>>>> topic, right?
> > >>
> > >>>>>
> > >>
> > >>>>>
> > >>
> > >>>>> Yes, non-durable subscriptions created by Reader, if the reader
> > >>
> > >>>>> disconnected, the subscription will be deleted automatically.
> > >>
> > >>>>>
> > >>
> > >>>>>
> > >>
> > >>>>>
> > >>
> > >>>>> For `NonPersistentDispatcher`, it has the following
> implementations:
> > >>
> > >>>>>
> > >>
> > >>>>> 1. NonPersistentDispatcherMultipleConsumers
> > >>
> > >>>>>
> > >>
> > >>>>> 2. NonPersistentDispatcherSingleActiveConsumer
> > >>
> > >>>>>
> > >>
> > >>>>> 3. NonPersistentStickyKeyDispatcherMultipleConsumers
> > >>
> > >>>>>
> > >>
> > >>>>>
> > >>
> > >>>>> For `NonPersistentDispatcherSingleActiveConsumer`, auto delete the
> > >>
> > >>>>> subscription when there isn’t consumer connected affect nothing.
> > >>
> > >>>>>
> > >>
> > >>>>>
> > >>
> > >>>>> But for `NonPersistentDispatcherMultipleConsumers`,
> > >>
> > >>>>> `NonPersistentStickyKeyDispatcherMultipleConsumers`, I’m not sure
> that
> > >> if
> > >>
> > >>>>> it matters when we auto delete the subscriptions which has no
> > >> consumers
> > >>
> > >>>>> connected.
> > >>
> > >>>>>
> > >>
> > >>>>>
> > >>
> > >>>>> If it doesn’t matter, I agree with you.
> > >>
> > >>>>>
> > >>
> > >>>>>
> > >>
> > >>>>> Auto delete the durable subscriptions when there aren’t consumers
> > >>
> > >>>>> connected to the non-persistent topic.
> > >>
> > >>>>>
> > >>
> > >>>>>
> > >>
> > >>>>>
> > >>
> > >>>>> Thanks,
> > >>
> > >>>>>
> > >>
> > >>>>> Tao Jiuming
> > >>
> > >>>>>
> > >>
> > >>>>>
> > >>
> > >>>>>
> > >>
> > >>>>>> 2023年2月7日 19:58，PengHui Li <peng...@apache.org <mailto:
> > >>
> > >>>>> peng...@apache.org>> 写道：
> > >>
> > >>>>>
> > >>
> > >>>>>>
> > >>
> > >>>>>
> > >>
> > >>>>>> I think we are talking about the durable subscription of the
> > >>
> > >>>>> non-persistent
> > >>
> > >>>>>
> > >>
> > >>>>>> topic, right? For the Reader API (non-durable subscription), the
> > >>
> > >>>>>
> > >>
> > >>>>>> subscription will be
> > >>
> > >>>>>
> > >>
> > >>>>>> removed after the reader disconnects. If not, it should be BUG.
> > >>
> > >>>>>
> > >>
> > >>>>>>
> > >>
> > >>>>>
> > >>
> > >>>>>> IMO, we should remove the subscription automatically if all the
> > >> consumers
> > >>
> > >>>>>
> > >>
> > >>>>>> are disconnected for the non-persistent topic. The consumers can
> only
> > >> get
> > >>
> > >>>>>
> > >>
> > >>>>>> the new incoming messages after they connect to the non-persistent
> > >> topic.
> > >>
> > >>>>>
> > >>
> > >>>>>> It looks like even if we leave the subscription there; it will
> > >>
> > >>>>>
> > >>
> > >>>>>> help nothing.
> > >>
> > >>>>>
> > >>
> > >>>>>> The behavior should be the same as creating a new subscription
> with
> > >> the
> > >>
> > >>>>>
> > >>
> > >>>>>> same subscription name.
> > >>
> > >>>>>
> > >>
> > >>>>>>
> > >>
> > >>>>>
> > >>
> > >>>>>> For subscription management, users are not able to disable the
> > >>
> > >>>>> subscription
> > >>
> > >>>>>
> > >>
> > >>>>>> auto-creation of the non-persistent topic? We don't have any
> metadata
> > >>
> > >>>>>
> > >>
> > >>>>>> persist
> > >>
> > >>>>>
> > >>
> > >>>>>> in the metadata store. After the broker restarts, we will lose
> > >>
> > >>>>> everything.
> > >>
> > >>>>>
> > >>
> > >>>>>>
> > >>
> > >>>>>
> > >>
> > >>>>>> So I think we don't need to introduce a new configuration.
> Instead, we
> > >>
> > >>>>>
> > >>
> > >>>>>> should
> > >>
> > >>>>>
> > >>
> > >>>>>> figure out if there is a strong reason to leave the durable
> > >> subscription
> > >>
> > >>>>>
> > >>
> > >>>>>> after
> > >>
> > >>>>>
> > >>
> > >>>>>> all the consumers are disconnected. If there is no strong reason,
> I
> > >> tend
> > >>
> > >>>>> to
> > >>
> > >>>>>
> > >>
> > >>>>>> fix
> > >>
> > >>>>>
> > >>
> > >>>>>> incorrect behavior directly.
> > >>
> > >>>>>
> > >>
> > >>>>>>
> > >>
> > >>>>>
> > >>
> > >>>>>> Best,
> > >>
> > >>>>>
> > >>
> > >>>>>> Penghui
> > >>
> > >>>>>
> > >>
> > >>>>>>
> > >>
> > >>>>>
> > >>
> > >>>>>>
> > >>
> > >>>>>
> > >>
> > >>>>>>
> > >>
> > >>>>>
> > >>
> > >>>>>>
> > >>
> > >>>>>
> > >>
> > >>>>>> On Tue, Feb 7, 2023 at 4:14 PM Jiuming Tao
> > >> <jm...@streamnative.io.invalid
> > >>
> > >>>>> <mailto:jm...@streamnative.io.invalid>>
> > >>
> > >>>>>
> > >>
> > >>>>>> wrote:
> > >>
> > >>>>>
> > >>
> > >>>>>>
> > >>
> > >>>>>
> > >>
> > >>>>>>> Hi,
> > >>
> > >>>>>
> > >>
> > >>>>>>>
> > >>
> > >>>>>
> > >>
> > >>>>>>> I’ve opened a new PIP to discuss:
> > >>
> > >>>>>
> > >>
> > >>>>>>>
> > >>
> > >>>>>
> > >>
> > >>>>>>> Currently, in Pulsar, we have a configuration named
> > >>
> > >>>>>
> > >>
> > >>>>>>> `subscriptionExpirationTimeMinutes` to manage the subscription
> > >>
> > >>>>> expiration
> > >>
> > >>>>>
> > >>
> > >>>>>>> of `PersistentTopic` and `NonPersistentTopic`.
> > >>
> > >>>>>
> > >>
> > >>>>>>>
> > >>
> > >>>>>
> > >>
> > >>>>>>> When we set a value which is greater than 0 to
> > >>
> > >>>>>
> > >>
> > >>>>>>> `subscriptionExpirationTimeMinutes`, it will affect both
> > >>
> > >>>>> `PersistentTopic`
> > >>
> > >>>>>
> > >>
> > >>>>>>> and `NonPersistentTopic`. Their inactive subscriptions will get
> > >> expired
> > >>
> > >>>>> and
> > >>
> > >>>>>
> > >>
> > >>>>>>> will clean automatically.
> > >>
> > >>>>>
> > >>
> > >>>>>>>
> > >>
> > >>>>>
> > >>
> > >>>>>>> For `NonPersistentTopic`, its subscriptions can be clean because
> we
> > >>
> > >>>>> don't
> > >>
> > >>>>>
> > >>
> > >>>>>>> guarantee its data integrity. But for `PersistentTopic`, if we
> clean
> > >> its
> > >>
> > >>>>>
> > >>
> > >>>>>>> subscriptions automatically may lead to data loss.
> > >>
> > >>>>>
> > >>
> > >>>>>>>
> > >>
> > >>>>>
> > >>
> > >>>>>>> However, their subscription expiration is managed by the same
> > >>
> > >>>>>
> > >>
> > >>>>>>> configuration(`subscriptionExpirationTimeMinutes`), we can't
> manage
> > >>
> > >>>>> their
> > >>
> > >>>>>
> > >>
> > >>>>>>> subscription expiration independently.
> > >>
> > >>>>>
> > >>
> > >>>>>>>
> > >>
> > >>>>>
> > >>
> > >>>>>>> So I want to introduce a new configuration named
> > >>
> > >>>>>
> > >>
> > >>>>>>> `nonPersistentSubscriptionExpirationTimeMinutes` to manage
> > >>
> > >>>>>
> > >>
> > >>>>>>> `NonPersistentTopic`'s subscription expiration.
> > >>
> > >>>>>
> > >>
> > >>>>>>>
> > >>
> > >>>>>
> > >>
> > >>>>>>>
> > >>
> > >>>>>
> > >>
> > >>>>>>> Please feel free to leave your comments, the PIP link:
> > >>
> > >>>>>
> > >>
> > >>>>>>> https://github.com/apache/pulsar/issues/19448 <
> > >>
> > >>>>> https://github.com/apache/pulsar/issues/19448> <
> > >>
> > >>>>>
> > >>
> > >>>>>>> https://github.com/apache/pulsar/issues/19448 <
> > >>
> > >>>>> https://github.com/apache/pulsar/issues/19448>>
> > >>
> > >>>>>
> > >>
> > >>>>>>>
> > >>
> > >>>>>
> > >>
> > >>>>>>>
> > >>
> > >>>>>
> > >>
> > >>>>>>> Thanks,
> > >>
> > >>>>>
> > >>
> > >>>>>>> Tao Jiuming
> > >>
> > >>>>>
> > >>
> > >>>>>
> > >>
> > >>>>>
> > >>
> > >>>
> > >>
> > >>
> > >>
> >
>
