-
Notifications
You must be signed in to change notification settings - Fork 688
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 status interface #843
Conversation
Signed-off-by: R.I.Pienaar <rip@devco.net>
Replicas() int | ||
|
||
// StreamName is the name of the stream used to store the data | ||
StreamName() 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.
This one I would prefer a bit more generic, I had it BackingStore
in the ADR. The idea is that once we have other backends that this would be some kind of pointer to the specific kind. So string might not be enough unless its a url, for jetstream at least we need to know stream name, domain, placement tags and placement cluster.
Initially I thought some kind of url structure, but not sure now, thoughts @derekcollison ?
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 might be tempted again here to not offer anything until we get more feedback and figure out replicas and how we want this to interop with other impls with other backends etc.
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.
We have the feedback, the CLI needs it hence the PR.
We need to be able to do things like backup the stream, restore etc, there are several quality of life things around buckets that is JS specific and where knowing the underlying stream name and location helps. We can ofcourse double up the implementation details, but thats no good.
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.
The naming transition from KV to Stream is well known and that is what is needed. The CLI can do that on its own no?
I am ok with making this more clearly aligned with JetStream, but I know you feel strongly about a generalized abstraction.
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 dont at this point realistically expect nats.go to get non js behaviors. We'd rather make a new one client library eventually that satisfies the interface (hence all the interface investment).
But for StreamName()
we can go back to BackingStore()
and have it return something like:
type BackingStore interface {
Kind string // jetstream, ngs, kine, etc
Store string // KV_FOO, depends on Kind what you put here
Options map[string]interface{} // domain:foo for example? obviously this can improve a bit, just showing the idea
}
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.
Possibly.. Or we do manager additions that can give you back a stream info for the KV and ObjectStores and leave the instance interfaces void to maximize compatibility. WDYT?
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.
Yes, I considered, but map[string]interface at least makes nice json by default:
type BackingStore interface {
Kind string // jetstream, ngs, kine, etc
Store string // KV_FOO, depends on Kind what you put here
Options interface{} // Kind specific types
}
So the first two support CLI info display and such, 2nd programatic things, if you know your tool supports Kind==JetStream
you can cast Options
to JS specific things which would be stream info and stuff like cluster info perhaps
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 think this is going to be most useful:
type BackingStore interface {
Kind() string // "JETSTREAM" etc
Info() map[string]string // domain:foo for example? kind specific stuff
}
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 am ok with Kind() (and can be string I guess versus enum) and some sort of map[string]string.
Not sure Options is the right thing to call it, but that is a harder problem ;)
TTL() time.Duration | ||
|
||
// Replicas is how many storage replicas are kept | ||
Replicas() int |
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 feel this is a bit simplistic - since we should communicate healthy and failed replicas to users who dont know/care for JS underlying specirfics. I had it Replicas() (healthy int, failed int)
but you didnt like that @derekcollison thoughts?
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.
Replicas needs a bit more thought imo, so would not rush to add that here just yet imo.
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.
CLI needs to be able to show the health of a bucket
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 understand but again I think we need to do some more ground work on mirrored replicas etc. Right now we do not show stream health based on mirrors AFAIK. I know we do report it though.
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.
cli shows it, healthy/unhealthy/lagged/offline etc, we can minimise that down to ok, bad int
or similar, thats enough for KV users to alert their storage/backend admins who understand the jetstream specifics
rolled into #845 |
Signed-off-by: R.I.Pienaar rip@devco.net