nats-io / nats-architecture-and-design

Architecture and Design Docs
Apache License 2.0
177 stars 20 forks source link

[update] ADR-8 prepare for stable 1.0 version KV #79

Closed ripienaar closed 2 years ago

ripienaar commented 2 years ago

This updates the ADR for recent discussions and states clearly that this is now a stable point.

A rough roadmap is included

Signed-off-by: R.I.Pienaar rip@devco.net

ripienaar commented 2 years ago

A note to the developers who were on the recent call arond the end-of-data marker.

When this was first designed a watch was going to send all data always, simple as that. So you will always reach Delta==0. Since then we had some scope creep - we can now ignore deletes, you want to send new only and so forth - this means that there are cases where we will never reach Delta==0 from the user perspective(it could be a delete), so you can never know if you reached end of data.

So we are back to requiring a end-of-data indicator, this is as per go implementation and the spec states that now.

scottf commented 2 years ago
  1. The Go impl sets deny_delete to true. Assuming this is correct. Can you add this in after line 242 ? after rollup_hdrs
  2. Did we remove the restriction for 64 entries per key?
  3. Are Create and Update (or put option) now formally required for 1.0?
  4. Are we adding support for colon in the key? If not I'll close my pr.
ripienaar commented 2 years ago
  1. The Go impl sets deny_delete to true. Assuming this is correct. Can you add this in after line 242 ? after rollup_hdrs

Not sure if strictly required, but for sure deleting random things will break watch so probably safe to deny it. Have updated.

2. Did we remove the restriction for 64 entries per key?

No was mentioned elsewhere, but called out specifically in the config section.

3. Are Create and Update (or put option) now formally required for 1.0?

Yes

4. Are we adding support for colon in the key? If not I'll close my pr.

No, I dont think so, adding : wont help them they have other needs we cant satisfy so other solutions needed.

scottf commented 2 years ago

Something we should consider and maybe add the decision to the ADR. For the watch with Deliver Policy new, I don't think we should ever send the data marker, my reasoning is as follows:

  1. If there are no entries for a watch key (pattern), the Last Message check won't find an entry and will send the marker. But the user asked for new. Do they want this marker?
  2. If there are 1 or more entries for a key. The Last Message check finds an entry and does not send the marker. Inside the [Go client] update function, there will eventually be a delta 0 at which time the code would send the marker. But again for new do we really want that behavior?
tbeets commented 2 years ago

Putting aside semantic difficulties with new in general, we should always send end-of the initial data set marker. This consistency will be easier to doc and grok and will help to avoid complexity as future options added that filter/influence the return in various ways.

tbeets commented 2 years ago

LGTM @ripienaar

tbeets commented 2 years ago

Recommend watch options and option names be called out explicitly for 1.0 to clarify MUST vs. MAY for the stable release. @ripienaar

scottf commented 2 years ago

@tbeets What would constitute the end of data for new? They could start a watch and not see an update for an hour, a day. They would get one or more data entries and then they would get a marker. It's not like default (last_per_subject) or history (all) where you are getting a bunch of messages right away or no messages at all. Either way it has to be documented and the current code doesn't have to change at all to always send the marker and there is minimal change to not send the marker on new, I already tried it on Java.

tbeets commented 2 years ago

@scottf I think new if given as a watch option would silently discard last_per_subject (or no-value if the key never written), then pop the end of initial data set marker. A program using the new watch option would see the marker every time as they set the watch (which they would ignore).

ripienaar commented 2 years ago

@tbeets What would constitute the end of data for new?

The marker is an End Of Initial Data marker.

With new you are at that point soon as the watch is operational. Ie. It’s the first thing across.

ripienaar commented 2 years ago

Watcher specification has been improved a bit to be clearer and to set expectations about naming etc.

scottf commented 2 years ago

With new you are at that point soon as the watch is operational. Ie. It’s the first thing across.

+1 As I was realizing that my proposal of not sending the marker wasn't popular, I had the same idea, to just send it right away, the first thing.

scottf commented 2 years ago

On updates. Is there any reason to allow them to not specify last revision and just update any existing?

ripienaar commented 2 years ago

On updates. Is there any reason to allow them to not specify last revision and just update any existing?

isnt that just a put then?

scottf commented 2 years ago

On updates. Is there any reason to allow them to not specify last revision and just update any existing?

isnt that just a put then?

Duh, right. Okay one more. Is it allowed to "update" a deleted key? You can know the revision based on a history call. Again, why would you as you can put, but should we allow it?

ripienaar commented 2 years ago

Duh, right. Okay one more. Is it allowed to "update" a deleted key? You can know the revision based on a history call. Again, why would you as you can put, but should we allow it?

Update is about consistency, I update a known previous state so that I know someone else did no mess around while I was doing something. Essentially ensuring a series of writes happen uninterupted.

So yes, we should allow update to deletes.

Create need some special handling: You try Update with 0, if that fails and the last is a delete, then you Update with revision of the delete.

And there you can see you need to support Update against the rev of a delete.

ripienaar commented 2 years ago

@matthiashanel Please suggest what changes you need for subjects, once this is merged you will not get your needs in for v1.

matthiashanel commented 2 years ago

Please include: As outlined in ADR 19, when an API prefix is specified, set it internally for JetStream and prefix every publish and subscribe with it.

ripienaar commented 2 years ago

Please include: As outlined in ADR 19, when an API prefix is specified, set it internally for JetStream and prefix every publish and subscribe with it.

I think its important to specify the KV feature in a single place rather than jump all over the show. I might be wrong but I think I gave that feedback before, so I really think we should put all relevant design details for KV in this ADR.