nats.go icon indicating copy to clipboard operation
nats.go copied to clipboard

Durable Consumer

Open typecampo opened this issue 2 years ago • 19 comments

The use of nats.Durable() in subopts passed to js.Subscribe() should be rejected.

There is a scenario where Durable is not Durable. If you call Drain or Unsubscribe on a Durable, the consumer is deleted. That is fair but consider this from the perspective of an administrator who does not know the code.

natscli reports the consumer to be durable by displaying its durable name. An administrator could safely assume a restart of a service that is a consumer will be fine and it will keep its position in the stream. The admin knows nothing of the code and does not know it contains a Drain or Unsubscribe. By restarting the service the admin unknowingly deleted and re-added the consumer.

I propose this solution, if nats.Durable() is passed in subopts then nats.Bind() should also be required. Or if Durable and Bind reports consumer not found, then error out. nats.Durable() should not be allowed to proceed if existing consumer not found. At the very least, the durable should report that it is not bound and may be deleted on Drain or Unsubscribe.

typecampo avatar Dec 15 '21 22:12 typecampo

I will research. I suspect the initial decision on silently removing an silently-created durable as part of js.Subscribe() was argument that a lot of silently-created durables would be left around after the application disconnects. But that begs the question of whether we should silently create durables or instead error if the named consumer (durable) does not exist at subscription time.

tbeets avatar Dec 15 '21 22:12 tbeets

In addition, what is the correct way to do a graceful restart if Drain deletes the durable? Maybe Flush would be better here. If so, then Durable should not be allowed with Drain? Perhaps separating the logic and let Drain drain. Or maybe DrainWithoutUnsub().

typecampo avatar Dec 15 '21 22:12 typecampo

By "restart" do you mean have your application client resume reading at a desired message sequence with an existing JS Consumer?

tbeets avatar Dec 15 '21 23:12 tbeets

Yes if I restart the application it should pickup where it left off before the restart. The drain would handle anything pending before application exits.

typecampo avatar Dec 16 '21 00:12 typecampo

@derekcollison The choice was made to have js.Subscribe() possible create the consumer, and also delete it. As you may remember, I was against this idea. Right now, we have explained to @typecampo that the library behaves as designed, but I am the first to say that this is not ideal.

Either we do a v2 where js.Subscribe() will never attempt to create (and therefore delete) durable JS consumers, and users will have to call AddConsumer() prior, which I know you don't want, or we introduce a new option that enable/disable auto-delete of the durable JS consumer. My preference would have been disable the auto-remove by default, but if we do that now, it is a change in behavior for users (if any) that expected the durable JS consumer to be removed on Unsubscribe()/Drain(). A case can be made that it is "ok" on Unsubscribe() since it could be understood as wanted to remove the interest, but clearly not on Drain(): users may want to drain pending messages before exit, but does not mean that they want to remove the interest.

kozlovic avatar Dec 16 '21 16:12 kozlovic

For some time before v1.13 Drain would not delete the consumer and only remove the interest, maybe just revert that part so that Drain does not delete the consumer?

wallyqs avatar Dec 16 '21 16:12 wallyqs

Didn't we agree to never delete any consumer that was not automatically made? Ie. it should never delete one bound to. I agree in general the design is not ideal but not deleting ones bound to seems like a good idea given the current state

ripienaar avatar Dec 16 '21 16:12 ripienaar

We don't delete the one we "bound" to.. But imagine this: no JS consumer currently exists. User calls js.Subscribe("foo", nats.Durable("dur")), the library will not find such durable and then call "add consumer", creating this durable. It "owns" it and on Unsubscribe()/Drain() will delete it. If such durable already existed, the same js.Subscribe() call will look-it-up, find it and will NOT attempt to delete it. I have documented that in the Go doc, but I really don't like this behavior..

kozlovic avatar Dec 16 '21 16:12 kozlovic

Ah, yeah you're right, that's not good - Durable must mean durable

ripienaar avatar Dec 16 '21 16:12 ripienaar

But it was decided that the library would work this way (see ADR-15). Again, I personally don't like it, but currently it works as designed. The question and reason we (@tbeets) asked Ryan to create this issue is to report how users would want the API to behave. Then we will see if we alter/v2 it, etc..

kozlovic avatar Dec 16 '21 16:12 kozlovic

The current behavior would work if nats.Durable meant only the name of the consumer. Perhaps, get rid of Durable and change to nats.Name in v2. Then nats.Durable could be combined with nats.Bind().

I dont understand the reasoning to allow the use of nats.Durable alone if it only ends up as an ephemeral consumer.

typecampo avatar Dec 16 '21 17:12 typecampo

A durable is different from ephemeral in that ephemeral is R1 only, while a durable can be Rn, which would allow to survive a node failure and durable would still operate.

kozlovic avatar Dec 16 '21 17:12 kozlovic

I suppose that would be ok since a failure would not have had the chance to call Drain. The consumer would not be deleted in this case.

What if Drain was changed to only close the connection instead of unsubscribe? As it is right now, you cannot use Drain without a pre made consumer or risk deletion.

typecampo avatar Dec 16 '21 17:12 typecampo

There are some implied use-case differences between "durable" and "ephemeral" I think that we should consider. Ephemeral seems best suited to 1 client's view of a stream (the client that knows the handle for that ephemeral it just created). Durable seems best suited to 1-or-more clients view of a stream that can rally around a pre-decided JS consumer name (i.e. there is a method/expectation of shared name). It follows: any time 1 (of possibly N) clients is creating a durable JS Consumer for an application need, it should do so explicitly, not implicitly, and any deletion should also be explicit.

tbeets avatar Dec 16 '21 20:12 tbeets

I'm just upgrading from v 1.11 and this is terrible( Thanks to @tbeets for mentioning this behaviour! If he didn't - all my server would go down under processing thousands and millions of events! Durable should be durable! I suggest implementing @kozlovic variant - remove implicit delete and add explicit option for removing durable on unsubscribe! For me it's nearly unimaginable case when you create "temporary" "durable" just for small amount of work and removing it immediately. FYI, My case: We have stream for unstructured events. Sometimes we add new service for processing particular kinds of events (we have several such services in each cluster). Service starts, subscribes for specific durable (creates if it doesn't exist) and processes events. We have multiple durable consumers and if @tbeets hadn't mentioned this - all our processing would have collapsed immediately along with aws budgets just because every service would process all events for last half year on each restart!

porfirion avatar Dec 16 '21 20:12 porfirion

Just my 2 cents that I agree with: if you create a durable (either explicitly, or automatically) then it should remain after you close Drain(), Unsubscribe(), Close(), etc... a 'durable' should only get deleted by explicitly deleting it (regardless of how it was created).

jnmoyne avatar Dec 17 '21 17:12 jnmoyne

We will improve.

The early rationale behind durables for single apps was around the network becoming unavailable. The app would create and destroy the consumer but the durable part was to survive both app failures and network or NATS server upgrades/failures.

Agree we have a situation though that is not desirable and violates path of least surprise.

derekcollison avatar Dec 17 '21 23:12 derekcollison

Any update?

jaekook avatar Jul 09 '22 13:07 jaekook

The simplification efforts are ongoing and the 2.9.0 release of the server will allow the evolution of the clients.

derekcollison avatar Jul 09 '22 23:07 derekcollison