search

nats-io / nats-architecture-and-design

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

Add GetRevision() #92

Closed kozlovic closed 2 years ago

kozlovic commented 2 years ago

GetRevision returns a specific revision value for the key, or "KeyNotFound" if that message for this revision can't be found. See https://github.com/nats-io/nats.go/pull/903

kozlovic commented 2 years ago

@ripienaar Not sure if this is needed in the ADR. Regardless, since we have added that to the Go client, I will create an issue so that other libs can add it.

ripienaar commented 2 years ago

I think to make it for the ADR

@ripienaar Not sure if this is needed in the ADR. Regardless, since we have added that to the Go client, I will create an issue so that other libs can add it.

I think to be suitable for the ADR we should consider rounding out the history feature set a bit - for example a API call to take revision x and promote it to the current value for a key.

I kept it out initially as I wanted to see how the history / audit feature shakes out. Could be fine to mark this as optional also and leave as is?

kozlovic commented 2 years ago

Could be fine to mark this as optional also and leave as is?

Just searched for "optional" in that whole PR and don't find it, so that would be the only one with that comment, so not sure this is the right approach then. Again, I have created the issue so that libraries can provide this feature, but maybe it is indeed optional and does not have to be in the ADR. In other words, happy to close this PR without a merge..

ripienaar commented 2 years ago

There's a few phrases like language implementations can add what makes sense in addition., essentially ADR is the core feature set but things like GetString(), GetBytes() etc exist in some - but not for everyone to adopt.

We can do that way or as you did, same same. I am not sure where we stand with stabalizing the API but imo the goal was we're not supposed to add new required things at this point - could list it in one of the future roadmap items above as a future delivery.

kozlovic commented 2 years ago

@ripienaar I removed from the kv-readonly section and added as a note where we say that we can add GetUint64() or the like. Feels a bit lost there and makes me really wonder if it is worth updating the ADR. Again, since this is considered optional, then I am not certain we need to update this, simply have the issue that I have created so that devs can implement it if they want their lib to have the API..

ripienaar commented 2 years ago

Yeah I agree, if its optional I wouldnt bother with the ADR. Later when we choose to add a set of audit/history related management things it makes sense in the ADR, in isolation its a bit low value

kozlovic commented 2 years ago

Closing then, since there is no value adding it there. Thanks!