add kv and object status functionality

[ripienaar]

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

[coveralls]

Coverage increased (+0.2%) to 85.064% when pulling 4b5b7cf on ripienaar:obj_status into 1655009 on nats-io:main.

[ripienaar]

avoids trying to parse pre to figure out domain which is not great at all

[ripienaar]

@derekcollison ok think I captured the outcome of our call, pls check it out

[derekcollison]

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?

[ripienaar]

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

[derekcollison]

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.

[ripienaar]

having status in KV being unlike the rest of KV is pretty confusing.

[ripienaar]

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%?

[derekcollison]

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.

[ripienaar]

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.

[derekcollison]

I can tell you feel pretty strongly about it. At the very least we need to make ObjectStoreStatus consistent.

Also might be good when you and I reach points like this to bring in others, so asking @kozlovic and @wallyqs to take a look as well.

[derekcollison]

LGTM

[kozlovic]

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..

[derekcollison]

Thanks that is what we did, just made ObjectStore the same.

[ripienaar]

fwiw, here's a use case for the interface.

Status is a minimal status implementation to maximise utility across different backends, however JetStreamStatus might also expose weird things like MaxBytes() and MaxValueSize(), PlacementCluster(), Domain() and StorageType(), these are JS specific and might not make sense in the context of lets say OtherKVBackend.

so

status, _ := bucket.Status()
// you always know what status is here and you can do basic things

// but if this is a JS backend, we have special features like read replicas, lets check and attached to a nearby one
if status.BackingStore().Kind() == "JetStream" {
   jsStatus := status.(*nats.JetStreamKVStatus)
   if jsStatus.PlacementCluster() != nc.ConnectedClusterName() {
      // set up a read replica
   }
}

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 Info() from BackingStore() and use this approach instead - now that interface stuff is resolved. Infact having resolved the interface question I'd go back to my initial suggestion of BackingStore() string that was in the ADR. Since you just dont need all the map[string]string, that's what interfaces are for.

It will avoid a bunch of meh code and API back and forths to the end user.