This is a tough one because the obvious ways of breaking an API are "URL format changes", "Request or Response format changes", etc. But I think the more obvious way to think about the API is, do the clients have to change their code? If the answer is yes, it's a breaking API change.
So, really the only way around this is to "default" the new field's value and make it optional, otherwise the API needs to rev. -Dew On Tue, Aug 15, 2017 at 8:09 AM, Jeremy Mitchell <mitchell...@gmail.com> wrote: > I don't think you can add a new not null column to the ds table because > that would break the DS api. For example, on DS create/update you are > saying routing_name is now required, right? This is an API breaking change > that would require a new version of the API, no? > > Hence my suggestion for a default. But, yes, i forgot about the per CDN > thing. > > Jeremy > > On Mon, Aug 14, 2017 at 5:25 PM, Peters, Rawlin <rawlin_pet...@comcast.com > > > wrote: > > > Jeremy’s suggestion could work, but the param would probably be created > in > > a TR_PROFILE per-CDN. However, that still wouldn’t fix the visibility > > problem. If a CDN isn’t using the default “tr” HTTP routing name, > operators > > would still need to know that there is a new profile parameter that needs > > updating post-upgrade but before a snap/queue. So either way there needs > to > > be sufficient upgrade notes, but personally I still prefer keeping the > > routing_name column non-null. > > > > That said, this is my current proposal for the DB migration which also > > gets us past the upgrade issue: > > 1. Add a routing_name column to the DeliveryService table. > > 2. Update the routing_name for DNS Delivery Services to “edge”. > > 3. Update the routing_name of non-DNS Delivery Services to the value of a > > temporary upgrade parameter associated with the Delivery Service’s CDN > (if > > the upgrade parameter doesn’t exist, the routing_names will remain null). > > 4. Update the remaining null routing_names to “tr”. > > 5. Make the routing_name column non-null and add a non-empty constraint. > > > > So these would be an operator’s pre-upgrade steps: > > 1. Verify if a custom http.routing.name has been configured for Traffic > > Routers in their CDNs. > > 2. If custom http.routing.name, do the following. Otherwise, no > > pre-upgrade steps needed (for per-DS routing names at least): > > a. create a parameter named “upgrade_http_routing_name” with the > value > > of the target CDN’s custom http.routing.name > > b. associate this parameter to the TR_PROFILE belonging to the target > > CDN > > c. repeat steps 2a and 2b for each CDN using a custom > > http.routing.name > > > > This would keep everything working the same post-upgrade as it did > > pre-upgrade, and from that point on you’d be able to change a Delivery > > Service’s routing name to any arbitrary hostname (without periods). > > > > --Rawlin > > > > On 8/14/17, 4:22 PM, "Dave Neuman" <neu...@apache.org> wrote: > > > > I don't think that solves the issue Rawlin was describing. The issue > > that > > Rawlin was describing is that someone has already defined a different > > routing name in traffic_router/http.properties which is no longer > > going to > > be used after the upgrade. The upgrade needs to somehow know about > > this > > other routing name and use that when it creates the database column > and > > populates it with the pre-defined defaults (edge, tr). > > > > Also, creating a global param doesn't help those with more than 1 > CDN. > > > > On Mon, Aug 14, 2017 at 4:09 PM, Jeremy Mitchell < > > mitchell...@gmail.com> > > wrote: > > > > > Adding a temp parameter would work but I worry that someone won't > > read the > > > upgrade documentation and forget to create this temporary parameter > > before > > > running the upgrade. > > > > > > Here's another option. > > > > > > Create 2 global TO parameters (http.routing.name and > > dns.routing.name > > > <http://http.routing.name/>) that default to tr and edge > > respectively and > > > make the ds.routing_name an optional field. > > > > > > in seeds.sql > > > > > > insert into parameter (name, config_file, value) values (' > > > http.routing.name', > > > 'global', 'tr') ON CONFLICT (name, config_file, value) DO NOTHING; > > > insert into parameter (name, config_file, value) values (' > > dns.routing.name > > > ', > > > 'global', 'edge') ON CONFLICT (name, config_file, value) DO > NOTHING; > > > > > > in code (warning. ugly pseudo code to follow): > > > > > > function getRoutingName(ds) { > > > return ds.routing_name if not null > > > if (ds.type = HTTP) { > > > return parameter.http.routing.name > > > } else > > > return parameter.dns.routing.name > > > } > > > } > > > > > > Just my 2 cents. > > > > > > On Mon, Aug 14, 2017 at 10:55 AM, Dave Neuman <neu...@apache.org> > > wrote: > > > > > > > Good info Rawlin. > > > > My vote would be for a parameter to be used during the upgrade. > > We can > > > set > > > > a param called `upgrade_routing_name` or something similiar so > > that it is > > > > obvious that it is a param used for upgrade only. We should also > > > document > > > > that A) the param needs to be set before upgrade and B) TR will > now > > > ignore > > > > the setting in the config file. Ideally we would remove the > param > > from > > > the > > > > default config and the need for the param in the code. > > > > > > > > On Wed, Aug 9, 2017 at 4:28 PM, Peters, Rawlin < > > > rawlin_pet...@comcast.com> > > > > wrote: > > > > > > > > > Hey all, > > > > > > > > > > I’ve dug through this a bit more, and defaulting a new > > > > > DeliveryService.routing_name > > > > > column to ‘tr’ for HTTP delivery services presents an upgrade > > issue if > > > a > > > > > CDN has > > > > > chosen to use a custom “http.routing.name” parameter for the > > Traffic > > > > > Routers > > > > > in that CDN (by editing the http.properties files of the > Traffic > > > > Routers). > > > > > > > > > > For instance, if “http.routing.name” has been set to “ccr”, > the > > new > > > > > routing name > > > > > “tr” will break all of the clients using the old “ccr” delivery > > service > > > > > URL. > > > > > > > > > > Basically we need to provide a one-time upgrade step to allow > > CDNs > > > using > > > > a > > > > > custom > > > > > “http.routing.name” to default the new routing_name column to > > that > > > value > > > > > for > > > > > existing HTTP delivery services. What would be the best way to > > do this? > > > > > Some options > > > > > might be: > > > > > 1. Add a profile parameter to the TR_PROFILE for that CDN. On > > upgrade, > > > > > read that > > > > > parameter and use it to update the routing_name for > existing > > HTTP > > > > > delivery services. > > > > > After upgrade, you can safely remove the profile parameter. > > > > > 2. Let the upgrade automatically default the routing_name to > > ‘tr’ or > > > > > ‘edge’. After > > > > > upgrading, manually update each HTTP delivery service to > use > > the > > > > > current > > > > > “http.routing.name” in use (we could provide an API > > endpoint to > > > > “bulk > > > > > update” the > > > > > routing names for all HTTP delivery services in a CDN). > > > > > > > > > > Note this is not an exhaustive list, this is a just a couple > > options > > > that > > > > > have come up in > > > > > discussion so far. Feel free to add any more ideas to this > list. > > > > > > > > > > After the upgrade has been completed, the “http.routing.name” > > > parameter > > > > > in the > > > > > Traffic Router’s “http.properties” file will be ignored (same > > with the > > > “ > > > > > dns.routing.name” > > > > > parameter in “dns.properties” which I’m not sure can even be > > changed > > > > > safely today). > > > > > > > > > > Thoughts? > > > > > > > > > > --Rawlin > > > > > > > > > > On 8/4/17, 10:19 AM, "Peters, Rawlin" < > rawlin_pet...@comcast.com > > > > > > wrote: > > > > > > > > > > @Dave @JvD > > > > > > > > > > Thanks for the feedback. I think I can get on board with > > defaulting > > > > > the DS columns to ‘edge’ and ‘tr’. I was thinking the CDN > > columns might > > > > be > > > > > useful if the user just wants to set it CDN-wide and not > > individually > > > on > > > > > each DS, but I guess if we default it as part of the upgrade > > migration, > > > > we > > > > > should also provide an API endpoint to set the routing names on > > all > > > DSes > > > > in > > > > > a CDN to a single value, thus still providing a “per-CDN” > option. > > > Would a > > > > > “bulk” update also be useful, in case a user wants to update a > > handful > > > of > > > > > DSes to the same routing names at once? > > > > > > > > > > @JvD Re: TR_PROFILE vs. DS_PROFILE > > > > > The default would come from a TR_PROFILE linked to the CDN, > > and the > > > > > override would come from a DS_PROFILE linked to that specific > > DS. I > > > > looked > > > > > into those options to cover all my bases, but I think adding > > columns to > > > > at > > > > > least the DS table and not touching profiles at all is the > better > > > option. > > > > > > > > > > -Rawlin > > > > > > > > > > On 8/4/17, 8:04 AM, "Jan van Doorn" <j...@knutsel.com> > wrote: > > > > > > > > > > Agree with Dave on > > > > > > > > > > [*DN] we should default the database column to "edge" > > for DNS > > > and > > > > > "tr" for* > > > > > *http. Then we don't have to do the null check.* > > > > > > > > > > If we do that, we can make the columns mandatory, and > it > > makes > > > > > sense > > > > > they're not in the DS_PROFILE. Also makes it so we > don't > > have > > > to > > > > > have a CDN > > > > > wide setting. (and Rawlin, I think you mean to say > > DS_PROFILE > > > > > rather than > > > > > TR_PROFILE type to add the param to if we chose to do > > that?? Or > > > > > was it the > > > > > default that goes into TR_PROFILE and the override into > > > > > DS_PROFILE?). > > > > > In any case - if we make the columns NOT NULL and > > default them > > > to > > > > > "tr" and > > > > > "edge", I'm +1 on columns in the deliveryservice table. > > > > > > > > > > > > > > > > > > > > Cheers, > > > > > JvD > > > > > > > > > > On Fri, Aug 4, 2017 at 7:12 AM Eric Friedrich > (efriedri) > > < > > > > > efrie...@cisco.com> > > > > > wrote: > > > > > > > > > > > Hey Rawlin- > > > > > > Zhilin has also been working on a very similar > > feature > > > which > > > > > was > > > > > > proposed on this mailer last month: > > > > > > https://lists.apache.org/thread.html/ > > > > > 51d7ed1ae65a3697c39edd00236e6f3897da37ef5b24ac452a17cabb@% > > > > > 3Cdev.trafficcontrol.apache.org%3E > > > > > > < > > > > > > https://lists.apache.org/thread.html/ > > > > > 51d7ed1ae65a3697c39edd00236e6f3897da37ef5b24ac452a17cabb@ > > > > > > <dev.trafficcontrol.apache.org>> > > > > > > > > > > > > Can you please work him to ensure we don’t duplicate > > work and > > > > > that if both > > > > > > solutions are needed they will work together? > > > > > > > > > > > > On Aug 3, 2017, at 3:57 PM, Peters, Rawlin < > > > > > rawlin_pet...@comcast.com > > > > > > <mailto:rawlin_pet...@comcast.com>> wrote: > > > > > > > > > > > > Sorry, Outlook converted my numbered list poorly. > I’ve > > > > corrected > > > > > the > > > > > > numbering (items 1-3) below. > > > > > > > > > > > > On 8/3/17, 1:52 PM, "Peters, Rawlin" < > > > > rawlin_pet...@comcast.com< > > > > > mailto: > > > > > > rawlin_pet...@comcast.com>> wrote: > > > > > > > > > > > > Hello All, > > > > > > > > > > > > I’ve been working on adding support for > configurable > > > per-CDN > > > > > and > > > > > > per-DeliveryService routing names [1] (what are > > currently > > > > > > hardcoded/defaulted to ‘edge’ and ‘tr’ for DNS and > HTTP > > > > Delivery > > > > > Services, > > > > > > respectively), and I have a few things to propose. > > > > > > > > > > > > > > > > > > 1. Add a column to the CDN table for the DNS > and > > HTTP > > > > > routing names. > > > > > > > > > > > > > > > > > > > > > > > > I’ve currently been working off the assumption > that > > > per-CDN > > > > > routing > > > > > > names would be configurable by adding ‘ > > http.routing.name’ > > > and > > > > ‘ > > > > > > dns.routing.name’ parameters to a profile of type > > TR_PROFILE > > > > > using the > > > > > > ‘CRConfig.json’ config file. To me this seems like > bad > > UX > > > > > because the user > > > > > > has to click through multiple steps and fill in > > multiple > > > fields > > > > > in the UI > > > > > > just to change the CDN’s routing names. It also > > requires > > > > joining > > > > > a few > > > > > > different tables in the DB just to find the > parameters > > > per-CDN. > > > > > For that > > > > > > reason, I think it would be better if > > ‘dns_routing_name’ and > > > > > > ‘http_routing_name’ were added as columns of the > ‘cdn’ > > table, > > > > > and changing > > > > > > them via the UI would follow the same process as > > choosing the > > > > > CDN’s domain > > > > > > name. Because the routing names would be the CDN-wide > > > defaults, > > > > > the ‘Edit > > > > > > CDN’ window feels like the most natural place to put > > it. > > > > > > > > > > > > > > > > > > 2. Values for per-DeliveryService routing names > > could > > > > live > > > > > in one of > > > > > > a couple different areas: > > > > > > * New columns in the delivery_service table > > > > > > * Parameters in a DS Profile > > > > > > > > > > > > As the developer, my vote would be for Option A > > because it > > > > > seems like > > > > > > it would lead to cleaner code in Traffic Ops because > > the > > > > routing > > > > > names > > > > > > would be readily-available when handling a > > DeliveryService. > > > You > > > > > wouldn’t > > > > > > have to also fetch its profile then dig through it to > > find > > > the > > > > > routing > > > > > > names. A downside could be that adding columns to an > > > > > already-overcrowded > > > > > > table isn’t ideal. > > > > > > > > > > > > Option B is less appealing to me but might have > some > > > > > advantages such as > > > > > > keeping the number of columns down in the > > DeliveryService > > > > table. > > > > > However, > > > > > > DS Profiles currently seem to be geared more towards > > the > > > > > Multi-site Origin > > > > > > feature in generating specific ATS configuration > > > > (parent.config) > > > > > and less > > > > > > towards a “junk drawer for optional config”. As the > > routing > > > > > names would > > > > > > affect the entire DS and multiple config files, it > > doesn’t > > > seem > > > > > right to > > > > > > have it as a profile parameter using ‘CRConfig.json’ > > as the > > > > > config file. I > > > > > > wasn’t around when DS Profiles were introduced, so if > > you are > > > > > more familiar > > > > > > with their purpose/origin and think this is a good > fit > > for > > > > them, > > > > > I’d like > > > > > > to hear your advice. > > > > > > > > > > > > > > > > > > 3. If per-DeliveryService routing names are not > > set > > > > > explicitly for a > > > > > > DS (i.e. the column is null), then the DS will use > the > > > per-CDN > > > > > routing > > > > > > names as a default. If the per-CDN routing names are > > unset, > > > > they > > > > > will > > > > > > default to the current values of ‘edge’ and ‘tr’. So > > the > > > lookup > > > > > hierarchy > > > > > > would be DS.routing_names -> CDN.routing_names -> > > default > > > > > ‘edge/tr’. > > > > > > > > > > > > I’d like to know what you think of these > proposals, > > and > > > any > > > > > > advice/feedback is welcome. > > > > > > > > > > > > Best regards, > > > > > > Rawlin > > > > > > > > > > > > [1] https://issues.apache.org/jira/browse/TC-287 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
