+1 A new requirement is a breaking change, needs to be a new major version, or a default value that doesn't break existing usage.
On Tue, Aug 15, 2017 at 8:40 AM, Dewayne Richardson <[email protected]> wrote: > 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 <[email protected]> > 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 < > [email protected] > > > > > 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" <[email protected]> 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 < > > > [email protected]> > > > 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 <[email protected] > > > > > 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 < > > > > [email protected]> > > > > > 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" < > > [email protected] > > > > > > > > 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" <[email protected]> > > 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) > > > < > > > > > > [email protected]> > > > > > > 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 < > > > > > > [email protected] > > > > > > > <mailto:[email protected]>> 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" < > > > > > [email protected]< > > > > > > mailto: > > > > > > > [email protected]>> 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
