Skip to content
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

Compact the versioning metadata #309

Merged
merged 1 commit into from
Nov 27, 2024
Merged

Compact the versioning metadata #309

merged 1 commit into from
Nov 27, 2024

Conversation

ripienaar
Copy link
Contributor

Also remove some unused information in ADR-43

Also remove some unused information in ADR-43

Signed-off-by: R.I.Pienaar <rip@devco.net>
|-------------------|-----------------------------------------------|
| `_nats.ver` | The current server version hosting an asset |
| `_nats.level` | The current server API level hosting an asset |
| `_nats.req.level` | The required API level to start an asset |
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to lower the impact without making it weird - like I am sure we could combine level and version in one entry but as discussed we need to solve the actual problem of info/config split rather than band aids.

So its smaller and fewer bytes and some fields removed but still obvious how it works.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can it just be one key like _nats.ver and the value is multiple values in an array that follows that ordering?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can but as above we want it to be meaningful and obvious to users what this is. Not needlessly obfuscate the design when that does not solve the problem - we are going to be adding more stuff to state and config. So this significantly reduce the byte size but balancing UX vs the byte count need

So we will try add a split to info/state APIs for 2.11 to improve the underlying cause of this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A design based on key ordering having magical value will get us the same place we are with the reply subject for stream msg metadata.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only on the wire, once in the client can be expanding out to separate keys / values no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The values are UX. it’s not an invisible feature that we hide its publically visible and answering “what is this?” On slack 500 times over is much more expensive than bytes on the wire.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree here with @ripienaar .
The API has to be clear and well defined.
If it's not, it causes bugs in clients and misunderstandings for users.
Especially as users will have access to it even if client do nothing about it.

Copy link
Member

@aricart aricart Oct 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about encoding the _nats entry as JSON, that would allow some of the keys to compact?

expanded: 63

 {"_nats.ver":"2.11.20","_nats.level":"0","_nats.req.level":"0"}

compact: 52

 {"_nats":"{\"ver\":\"2.12.1\",\"api\":1,\"min\":0}"}

We could also remove 8 more bytes, if we had a struct that managed these values outside of metadata, since these are system values.... (these are the escapes by doubly encoding JSON)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems pretty awful and additional overhead with json encoding.

They could be struct keys but I avoided that since I think we'll add more over time and want a bit of flexibility here without churning all the clients and data structs

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

standalone:

 {"ver":{"srv":"2.12.1","api":1,"min":0}}

Copy link
Member

@MauriceVanVeen MauriceVanVeen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

adr/ADR-43.md Show resolved Hide resolved
derekcollison added a commit to nats-io/nats-server that referenced this pull request Nov 27, 2024
Based on
nats-io/nats-architecture-and-design#309

Compact the keys and remove the 'created' keys.

Signed-off-by: Maurice van Veen <github@mauricevanveen.com>
@ripienaar ripienaar merged commit 3886107 into main Nov 27, 2024
1 check passed
@ripienaar ripienaar deleted the compact_versioning branch November 27, 2024 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants