New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add kv and object status functionality #845
Conversation
@@ -214,6 +214,8 @@ type jsOpts struct { | |||
aecb MsgErrHandler | |||
// Maximum in flight. | |||
maxap int | |||
// the domain that produced the pre | |||
domain string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
avoids trying to parse pre to figure out domain which is not great at all
@derekcollison ok think I captured the outcome of our call, pls check it out |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that things are becoming more consistent between the two. Like the concept of backing store.
I still would like to see more consistency between KV and Object for Status. I think a plain struct might suffice here vs the interface and private concrete types and would be more consistent with StreamInfo.
These can be assembled by any impl just as well as the interfaces, so I am struggling to see the benefit of the extra work to make status be an interface. WDYT?
being an interface allows for optimisations - compute things only when asked to compute it etc, not used here, but I dont really see a drawback in interfaces for KV since it makes KV consistent with KV. Later a KV implementation (I have a toy cache for example) could live outside of nats.go and just do whatever it likes, thats the point of interfaces and with a vague goal of pluggability for KV I thnk its important, and really not much work at all. For KV vs Object I flattened the Object store to show what it want as a struct - again consistent with rest of object store |
I am not sure that buys us anything when asking about the status (optimization), and feel that there should be some consistency between StreamInfo, KVStatus and ObjectStoreStatus. |
having status in KV being unlike the rest of KV is pretty confusing. |
Again, the point of interfaces is to have different implementations. A status object from another backend/cache/something might have entirely other needs and might wish to store additional things inside it, and then in addition implement the kv status interface. It's pretty critical for multiple implementations and why the entire KV has done this. Why give up in the last 1%? |
I agree regarding interfaces in general, but on this one the values are simple and read-only, we are just creating unnecessary work for ourselves imo. Is there a direct use case that you are thinking of that can not form a simple status struct like stream info for KV? All impls will produce same simple struct imo. |
What extra work? The work is done just say LGTM. Extra work is undoing all this now to make less work somehow. Every aspect of KV is an interface. It’s pointless to have this one thing not be an interface. If you want consistency I will make the object one also be a function based approach. It’s not about immediate use cases. It’s about future proofing. A lot of go code features just do not work with bare structs like generics for example will only support things interfaces can describe. NOT using an interface now close the door to many future directions while using tbem takes away nothing. Please approve it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Signed-off-by: R.I.Pienaar <rip@devco.net>
I agree with @ripienaar that the work is already done (interface/implementation) so removing interface would probably more work, but more importantly, let's keep interfaces so that it allows for future extensions or for user/tests to mock, etc.. |
Thanks that is what we did, just made ObjectStore the same. |
fwiw, here's a use case for the interface. Status is a minimal status implementation to maximise utility across different backends, however so
Here we use Kind() to trigger on special JS only behaviors thats optional. If we had only structs we'd have to do essentially this map[string]string thing, so personally I am inclined to drop the It will avoid a bunch of meh code and API back and forths to the end user. |
Signed-off-by: R.I.Pienaar rip@devco.net