Add a mechanism for converting SQL schemas based on negotiated version #2417
Conversation
jackkleeman
force-pushed
the
sql-schema-converters
branch
from
9c3dbe8 to
81791f3
Compare
There was a problem hiding this comment.
Thanks a lot for repairing support for older clients @jackkleeman. The changes look good to me. The two relevant questions I had were whether we settled on encoding the version via header values and whether no header value should mean the latest version. For the latter, I can see that is very convenient when starting (because you don't have to think about versions) but once you upgrade, you might be bitten by it. Is this a common practice that people would be aware of?
Regarding whether the CLI 1.2 should be able to talk to a server 1.1: It would certainly be nice but I am unsure about the added complexity at this point. Having a meaningful error message telling users what to do might be good enough. I guess where it becomes inconvenient is when one is interacting with different Restate server versions (e.g. local dev is 1.2 and production is 1.1).
| AdminApiVersion::HEADER_NAME, | ||
| http::HeaderValue::from(value.as_repr()), |
There was a problem hiding this comment.
Did we come to a conclusion how to encode the different versions (header vs. path)?
| match headers.get(Self::HEADER_NAME) { | ||
| Some(value) => match value.to_str() { | ||
| Ok(value) => match value.parse::<u16>() { | ||
| Ok(value) => match Self::try_from(value) { | ||
| Ok(value) => value, | ||
| Err(_) => Self::Unknown, | ||
| }, | ||
| Err(_) => Self::Unknown, | ||
| }, | ||
| Err(_) => Self::Unknown, | ||
| }, | ||
| // CLI didn't used to send the version header, but if we know its the CLI, then we can treat that as V1 | ||
| None if is_cli => Self::V1, | ||
| None => Self::Unknown, | ||
| } |
There was a problem hiding this comment.
Personally, I am not a huge fan of this kind of special casing because it can be confusing.
Is it standard practice to consider a request w/o a version header as the latest? While this is very convenient, it seems also like the best way to run into breaking changes because initially users don't have to think about versions until they upgrade.
Would it be an option to always consider a missing version header as v1?
There was a problem hiding this comment.
i think it is not standard practice. i think if its a header, we need to make sure that the naive curl case without the header does something sensible. if its a url path, we could perhaps deprecate the unversioned one, but given that we are versioning the whole api, it could mean fairly regularly breaking old scripts/clients which use a versioned url, when we remove support for an old api version. i don't really have a great answer here!
There was a problem hiding this comment.
Yeah, the versioning approach via headers makes it a bit more hidden and I agree that the basic curl case should still work. At the same time, I would like to avoid a very obvious foot gun if possible.
Regarding deprecating a versioned url, I wouldn't worry too much about it. This is something that we can announce and then slowly phase out to give users time to adjust. This won't be so easily possible for the API that always serves the latest version (unless it is announced in advance).
Co-authored-by: Till Rohrmann <[email protected]>
Currently there are 2 ways that old CLIs will break against 1.2
And i want to introduce a new way, by encoding our timestamp data correctly instead of as date64.
To cover these 3 breaking changes, I have added a mechanism to convert schemas on the fly based on negotiated version. CLI clients will start providing their negotiated version as a header
x-restate-admin-api-version. CLI user-agent clients that don't do so will be presumed to be on version 1. Other clients that don't do so will be presumed to be happy with the latest version. This is still a slight breaking change if someone is depending on certain schema types, but they can providex-restate-admin-api-version: 1to maintain the current behaviour.NB: this PR does not cover newer CLIs talking to older restate clusters. In my opinion we should move the CLI to use JSON for the sql operations, which is itself a somewhat breaking change as json is only present from 1.1.3, but once thats sorted, these conversions become mostly irrelevant for new CLIs.