Without actually seeing how that would look across the code base, the
best I can say is maybe. On the surface your proposal seems to improve
the areas I'm concerned about, but we still have this implicit model
where the server is responsible for dealing with older clients that
might not submit all data as expected. This implicitly requires us to
handle the absence of that data in future APIs and think about how any
change might impact all client versions across versions of ATC.
My concern really amounts to the investment of time required to think
through and implement changes that may affect the myriad of different
client/server version combinations. If we remove that from the
equation entirely, we have a much simpler API that has a 1:1
correspondence with the route and function, and only one way to
create/update a $thing (i.e.: a delivery service). I think having only
one way to create/update a $thing is a much safer way of doing
business than continuing to support multiple versions of clients,
regardless of how easy that might be with this proposed approach.
Unless I'm missing something, the implementation might be simplified
using this approach but the complexity of solving for the combination
of client versions still exists which makes it harder to do anything
when writing API code.
So, it isn't a matter of whether this approach is simple enough for us
to continue with semantic versioning. It's a matter of whether we want
to have to continue to deal with older clients that prevent us from
making certain changes in the API because we are afraid of breaking
that client. I think that's a lot of burden for our small development
team to shoulder for questionable utility. Viewed from another lens,
with the semantic versioning approach we are enabling clients to be
lazy about updating their _unknown and custom_ client code at the cost
of developer productivity and progress on our project.
I'm not saying that semantic versioning is solely to blame for our
lack of progress on our migration to Golang, but it's one more thing
that is slowing us down and definitely hasn't helped improve progress.
--
Thanks,
Jeff
On Thu, Apr 18, 2019 at 3:23 PM Robert Butts <r...@apache.org> wrote:
>
> >This is about simplifying our code in the API
>
> @jeff.elsloo That's what the tag solution I proposed does. The only
> difference from not versioning, is that fields will have a new tag,
> "NewField *int `json:"newField, db:"new_field", api:"1.5"`, and endpoints
> will have an extra line, "json := api.NewJSON("1.4")". That's it. That
> would be the entirety of the API code (or very nearly, Rawlin is right, I
> haven't implemented it to be 100% sure). The library itself is also tiny,
> it's ~250 lines of logic in a single file. 500 lines including comments and
> boilerplate.
>
> How do you feel about that? Would that be simple enough?
>
>
> On Thu, Apr 18, 2019 at 3:15 PM Fieck, Brennan <brennan_fi...@comcast.com>
> wrote:
>
> > >If you're deploying the head of master, API minor versioning doesn't
> > really solve that consistent API problem unless we start saying that
> > every single new field added to an API endpoint is a new minor version
> > instead of just incrementing an API's version once per TC release.
> >
> > Yeah, you shouldn't expect an active development branch to be stable -
> > it's the whole reason we have releases at all. We can't support something
> > that changes potentially a dozen times in a day.
> >
> > >If someone goes
> > to the trouble to understand how our APIs work and develops their own
> > client code, why is it so unreasonable to expect them to also
> > understand how an update of Traffic Ops could impact their _custom_
> > tooling?
> >
> > I agree with this so hard. I'd love to just say "TO vX uses the vX API,
> > major changes to the biggest TC component are a major change to TC", but at
> > any given time we support and provide bug/security fixes for versions X and
> > X-1. I'll settle for eliminating minor API versions, though. Developers can
> > be expected to understand that changing versions of a thing can change
> > aspects of the ways in which you can interact with said thing. A major
> > version change means major changes and a minor version change means minor
> > changes.
> > ________________________________________
> > From: Jeff Elsloo <jeff.els...@gmail.com>
> > Sent: Thursday, April 18, 2019 2:59 PM
> > To: dev@trafficcontrol.apache.org
> > Subject: Re: [EXTERNAL] Re: Traffic Ops API versioning issues
> >
> > > Maybe I'm the only one, and everyone else can vote me out, but I don't
> > see
> > that as acceptable. It's our responsibility as developers to create a safe
> > user experience, and unacceptable to declare real bugs to be the user's
> > fault for not using it right. When our Production CDN goes down because an
> > Ops person used an old client and didn't "just recompile," it's not that
> > Ops person's fault, it's our fault as Developers, for designing a dangerous
> > system. Our job is to prevent the CDN from going down, not to shift the
> > blame when it does.
> >
> > I don't think this is about shifting blame, safety, or the potential
> > to crash a CDN. This is about simplifying our code in the API and
> > making it more maintainable. If we simplify the API, we can accelerate
> > development and get more things done, and maybe even complete this
> > Golang migration. Another plus is simplification of routes by
> > eliminating versioning means less code and likely more stability and
> > safety, easier testing, and less developer confusion, in the long run.
> >
> > I think it's unreasonable for us to shoulder the burden and cost to
> > maintain various API versions because we're afraid we might break some
> > client out in the wild that might or might not exist. If someone goes
> > to the trouble to understand how our APIs work and develops their own
> > client code, why is it so unreasonable to expect them to also
> > understand how an update of Traffic Ops could impact their _custom_
> > tooling?
> >
> > Obviously we have to hold up our end of the deal and have good API
> > documentation and change logs. I think the cost of maintaining that is
> > much less than API versioning given our experience, especially after
> > we simplify the APIs. We're already doing much of that today.
> > --
> > Thanks,
> > Jeff
> >
> > On Thu, Apr 18, 2019 at 11:37 AM Robert Butts <r...@apache.org> wrote:
> > >
> > > >Without minor versions, #3497 would not even an issue. It's only an
> > issue
> > > because of the attempt to support minor versioning.
> > >
> > > That's simply not true. It's exactly the same issue. Removing minor
> > > versioning just hides the issue. You have declared:
> > >
> > > >only certain clients that don't handle new unknown fields would
> > > potentially be broken
> > >
> > > >all the client has to do is recompile
> > >
> > > 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.
> > >
> > > 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.
> > >
> > > Maybe I'm the only one, and everyone else can vote me out, but I don't
> > see
> > > that as acceptable. It's our responsibility as developers to create a
> > safe
> > > user experience, and unacceptable to declare real bugs to be the user's
> > > fault for not using it right. When our Production CDN goes down because
> > an
> > > Ops person used an old client and didn't "just recompile," it's not that
> > > Ops person's fault, it's our fault as Developers, for designing a
> > dangerous
> > > system. Our job is to prevent the CDN from going down, not to shift the
> > > blame when it does.
> > >
> > > >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?
> > >
> > > >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. Notwithstanding, the tag library already has 90%
> > > test coverage and 3x as many lines of test code as logic; and the API
> > Tests
> > > are actually pretty easy, I just added one in the old-version-update fix,
> > > and it was much easier than I expected:
> > >
> > https://github.com/apache/trafficcontrol/pull/3500/commits/16f2c96f086836f1d655fd62e673ee0a5e95e785
> > > .
> > >
> > > >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
> > >
> > > I believe it is easy. The function to parse tags can use the tags in the
> > > primary object (Cachegroup), and the sub-objects (LocalizationMethods)
> > will
> > > have their own version tags. I could be mistaken, I haven't actually
> > > written the code yet, but I'm pretty sure sub-objects with sub-updates
> > > won't be any more difficult or require much if any special logic.
> > >
> > >
> > > On Thu, Apr 18, 2019 at 10:37 AM Gray, Jonathan <
> > jonathan_g...@comcast.com>
> > > 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" <rawlin.pet...@gmail.com>
> > 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
> > > >
> > > >
> > > >
> >
