On Thu, Apr 18, 2019 at 11:37 AM Robert Butts <[email protected]> wrote: > Something doesn't cease to be an issue, because you redefine it to be the > user's fault. It's exactly the same issue, removing minor versions just > makes it much more difficult to debug.
It's not the user's fault, it's the faulty client's fault. If the client handled JSON for what it is instead of having to parse the JSON into statically-defined types with specific fields, dropping everything from the JSON that it isn't aware of, then it wouldn't be a problem for the client. In our repo only the Go client has that issue, and I'm sure we could figure out a way to get it to work if we really wanted to. IMO recompiling the Go client is not all that much to ask per release. > You're proposing not only removing minor versions, but creating data loss > and version mismatch bugs, and declaring them to be the user's fault. No, faulty minor versioning code is _currently_ causing data loss, and that has nothing to do with the user. I'm proposing to keep the code simple so that we're less likely to create bugs in the future, which in the end is even better for the user. We already have enough bugs and features to worry about without having to worry about all the extra minor versioning stuff. > >Switching all the endpoints over to your "apiver" library would not be as > trivial to implement or remove as you make it sound. > > Maybe. I'm offering to do it. If you're sure, why don't you let me > demonstrate, and prove myself wrong? There's nothing stopping you from demonstrating in your own fork, but I'd say we'd have to agree as a community to support TO API minor versioning from here on out, with very clear guidelines for what that truly means, before merging anything into the codebase. I don't think we can "try out minor versioning for a bit" then pull it out later if we don't like it. Between those two points in time, people will implement new features on top of the minor versioning stuff, and will be impossible to remove without affecting everything again. > >It would require lots of added API test coverage > > Require? That would be ideal, but we have supported minor versions for the > history of Traffic Ops, and never had extensive version tests. I agree we > should, but you're adding additional requirements to further your position, > which doesn't seem fair. I don't think it's unfair to ask for automated testing of a new feature that would require nontrivial modifications to all API endpoints. And I believe that's what this would be -- a new feature. I've only been a part of this project for a couple years now, but it has never seemed to me like Traffic Ops has _ever_ supported minor versioning the way it's supposed to work up until some efforts the Golang rewrite which were not well communicated or understood. To me it seems like there has always just been "a minor version in the TO API version", but we've never made "real minor versioning" a first-class citizen of the TO API. I've added a handful of new fields to the TO API in the last two years, and not once was I ever told that those new fields should require a new minor version (with all the proper handling we've discussed here) up until the recent efforts in the deliveryservice API Golang rewrite. Basically all I've been told and have been telling others is that: - you can't add new required fields to an API. New fields always have to be optional and have a default value. - you can't remove existing fields from an API without some deprecation cycle If we follow those two tenets, we support the "major version promise" as I've discussed before, with minimal client impact. No software is perfect, and there are always tradeoffs. - Rawlin > On Thu, Apr 18, 2019 at 10:37 AM Gray, Jonathan <[email protected]> > wrote: > > > At the end of the day, what I want is a consistent API that I can code > > against in the head of master that's treated like a contract. As an API > > user outside of the ATC repo it's incredibly frustrating to have my stuff > > break all the time. It basically encourages never developing using the > > latest API versions (regardless of how they're defined and even then things > > still break retroactively) or a non-official OSS release alltogether. It's > > a catch22 to be forced to either not vendor the go/python/bash libraries > > which leads to constant develop/recompile/deploys in lockstep with ATC or > > vendor and still have to do these things when stuff breaks anyway in the > > API. Really debating the native client libraries at all is just a red > > herring because the root issue is the HTTP API itself which is the real > > thing to care about since not all integrations use one of the client > > libraries, nor can be forced to do so, and may require a rigid API > > definition. > > > > Jonathan G > > > > > > On 4/18/19, 10:12 AM, "Rawlin Peters" <[email protected]> wrote: > > > > > The UPDATE statements need modified to fix #3497 even if we get rid > > of > > > versioning. Unless we decide to permanently break all clients older > > than > > > the newest server field, with every new server upgrade. The only > > other > > > option is to fix the updates. Unless you know of a way to fix missing > > > fields without changing the update statements, that I'm not seeing? > > > > By removing minor versioning, only certain clients that don't handle > > new unknown fields would potentially be broken, and I believe only the > > TO Go client has that problem in our repo. However, the TO Go client > > happens to use the same Go structs as traffic_ops_golang, so whenever > > new fields are added to the API, all the client has to do is recompile > > with the up-to-date structs. Unless we made breaking changes to the > > client, in most cases all that would be needed for those clients is a > > recompile. Traffic Portal, the Python TO client, and I'm pretty sure > > the Java TO client all handle unknown fields properly. > > > > Without minor versions, #3497 would not even an issue. It's only an > > issue because of the attempt to support minor versioning. If we just > > support the major version, all client requests would be treated as v1, > > and there would only ever be one SQL UPDATE statement per major > > version. We wouldn't need to "upgrade" 1.2 requests into a 1.4 struct > > (thus preventing the bug in #3497) by selecting and inserting all 1.4 > > values from the DB into the struct before handling the request or > > dynamically generating the SQL UPDATE statement to use based on the > > requested minor version. > > > > > So, this solution actually gives us > > > this bug fix almost for free. All that's required is another small > > function > > > to iterate over the object fields to create the update query. It's > > by far > > > the easiest and simplest fix for #3497; unless we also permanently > > break > > > all older clients on every server upgrade along with the minor > > version > > > removal. > > > > Switching all the endpoints over to your "apiver" library would not be > > as trivial to implement or remove as you make it sound. It would > > require lots of added API test coverage and a non-trivial amount of > > code modifications to all API endpoints. Certain UPDATE queries might > > be easy to generate from a given struct if the struct only uses a > > single table, but I don't think something like that would work for a > > field like `cachegroup.LocalizationMethods` which doesn't come from > > the cachegroups table and is updated separately from the rest of the > > cachegroup fields. > > > > - Rawlin > > > > > >
