+1 on setting the default in the DB. Also, +1 on doing the right thing in the code like Jeremy is describing.
On Tue, Aug 15, 2017 at 10:06 AM, Jeremy Mitchell <[email protected]> wrote: > basically what i'm saying is if you are going to keep routing_name as a non > null column you are going to have to handle it's potential absence in the > api layer....i think... > > jeremy > > On Tue, Aug 15, 2017 at 10:02 AM, Jeremy Mitchell <[email protected]> > wrote: > > > thb i'm not 100% sure how db defaults work but i think in the DS api > > create/update since routing_name is going to be optional, you'll want to > > see if it exists (and not "") in the request json and if so, use that > value > > to update the db else retrieve the default (from wherever you decide) and > > use that value to update the db. > > > > jeremy > > > > > > > > On Tue, Aug 15, 2017 at 9:14 AM, Peters, Rawlin < > [email protected] > > > wrote: > > > >> How do we feel about setting a default for the column in the DB schema? > >> The routing name can be any arbitrary hostname (without periods) after > this > >> support is added, and if someone doesn’t like the default we choose, > they > >> can always provide/update their own routing_name via the UI/API. > >> > >> --Rawlin > >> > >> On 8/15/17, 8:42 AM, "Robert Butts" <[email protected]> wrote: > >> > >> +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/ > >> > > > > > > 51d7ed1ae65a3697c39edd00236e6f > >> 3897da37ef5b24ac452a17cabb@% > >> > > > > > > 3Cdev.trafficcontrol.apache.org%3E > >> > > > > > > > < > >> > > > > > > > https://lists.apache.org/thread.html/ > >> > > > > > > 51d7ed1ae65a3697c39edd00236e6f > >> 3897da37ef5b24ac452a17cabb@ > >> > > > > > > > <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 > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > > >> > > > > > > > >> > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > > >> > > > > >> > > > >> > > >> > >> > >> > > >
