I've also created an issue ( https://github.com/apache/incubator-trafficcontrol/issues/1543) and subsequent PR (https://github.com/apache/incubator-trafficcontrol/pull/1548) to address the different requirements of each type of DS.
On Wed, Nov 15, 2017 at 3:10 PM, Jeremy Mitchell <mitchell...@gmail.com> wrote: > after thinking about this a bit more, I think I will just leave dscp, > regional_geo_blocking and routing_name as NOT NULL in the database. it's > probably easier to work around those requirements than removing the > constraint and having to deal with the fallout of that. > > jeremy > > On Wed, Nov 15, 2017 at 2:02 PM, Jeremy Mitchell <mitchell...@gmail.com> > wrote: > >> I can buy that. I'll leave the NOT NULL on routing_name alone. And yes, I >> agree, ANY_MAP seems to be quite an oddball. >> >> On Wed, Nov 15, 2017 at 1:10 PM, Rawlin Peters <rawlin.pet...@gmail.com> >> wrote: >> >>> I'm not a big fan of the ANY_MAP type DeliveryService. This is from >>> the documentation (basically the only information about this type): >>> >>> """ >>> ANY_MAP is not known to Traffic Router. For this deliveryservice, the >>> “Raw remap text” field in the input form will be used as the remap >>> line on the cache. >>> """ >>> >>> That statement is false because if an ANY_MAP DS is set to active, it >>> will make it into the CRConfig and be treated as an HTTP DS by the >>> Traffic Router [1]. But when it's inactive, it's "Raw remap line" >>> still gets deployed to its assigned caches remap.config [2]. I think >>> the only problem it was trying to solve was adding special lines to >>> remap.config for certain sets of caches, so I believe any TR-specific >>> fields of a DS don't apply to the ANY_MAP type, i.e.: >>> >>> - active (because it should always be false yet still ends up in >>> remap.config?) >>> - ccr_dns_ttl >>> - geo_limit >>> - geo_limit_contries >>> - geo_provider >>> - geolimit_redirect_url >>> - regional_geo_blocking >>> - tr_request_headers >>> >>> For the routing_name column, if we remove the NOT NULL constraint, >>> then we still have the problem of interpreting NULL as some string, >>> and it's much easier to keep it NOT NULL and make sure a default gets >>> inserted in the API layer rather than sprinkle an "if undefined use >>> 'blah'" guard all over the codebase. So, if we removed the NOT NULL >>> constraint, I think we'd still have to keep enforcing that constraint >>> in the API layer so that no null values end up in the routing_name >>> column. >>> >>> - Rawlin >>> >>> [1] https://github.com/apache/incubator-trafficcontrol/issues/1121 >>> [2] https://github.com/apache/incubator-trafficcontrol/issues/905 >>> >>> >>> On Wed, Nov 15, 2017 at 11:07 AM, Jeremy Mitchell <mitchell...@gmail.com> >>> wrote: >>> > I agree with you 100%, rob butts but i'd rather not tackle that big of >>> a >>> > database refactor at the moment. :) >>> > >>> > In the short term, I'd like to remove the NOT NULL database constraints >>> > from the columns that don't pertain to ALL delivery services. Sadly, >>> > overloading the ds table removed the ability to perform validation at >>> the >>> > database level which I think is actually pretty important but I guess >>> we >>> > can use the API/UI level validation for the time being. >>> > >>> > Jeremy >>> > >>> > >>> > >>> > On Wed, Nov 15, 2017 at 11:00 AM, Robert Butts < >>> robert.o.bu...@gmail.com> >>> > wrote: >>> > >>> >> If the field only applies to some DS types, it should be in its own >>> table. >>> >> Then it can be NOT NULL in that table, ideally with a constraint >>> requiring >>> >> the DS to be a type it applies to. You wanted to reduce the columns >>> in the >>> >> `deliveryservice` table, right? Here's your chance :) >>> >> >>> >> On Wed, Nov 15, 2017 at 10:56 AM, Jeremy Mitchell < >>> mitchell...@apache.org> >>> >> wrote: >>> >> >>> >> > From what I can tell, there are 4 flavors of delivery services: >>> >> > >>> >> > 1. DNS* >>> >> > 2. HTTP* >>> >> > 3. STEERING* >>> >> > 4. ANY_MAP >>> >> > >>> >> > And the properties associated with each flavor vary so I put >>> together a >>> >> > spreadsheet to show which properties pertain to each flavor: >>> >> > >>> >> > https://docs.google.com/spreadsheets/d/1rb4WzP1FicumyU1z6o_ >>> >> > wasxD0ih1HoSH3bT-IA1GSlw/edit?usp=sharing >>> >> > >>> >> > First things first, anything look wrong there? Have I >>> misrepresented / >>> >> > misunderstood anything? >>> >> > >>> >> > Things that jump out at me: >>> >> > >>> >> > 1. dscp CANNOT be null yet it only applies to 2/4 of the delivery >>> service >>> >> > types (DNS* and HTTP*) >>> >> > 2. regional_geo_blocking CANNOT be null yet it only applies to 2/4 >>> of the >>> >> > delivery service types (HTTP* and ANY_MAP) >>> >> > 3. routing_name CANNOT be null yet it only applies to 3/4 of the >>> delivery >>> >> > service types (DNS*, HTTP* and STEERING*) >>> >> > 4. What's up with ssl_key_version? Is this even used? I don't see >>> it in >>> >> the >>> >> > TO UI or the TP UI. >>> >> > >>> >> > Suggestions: >>> >> > >>> >> > 1. we remove the NOT NULL constraint from dscp, >>> regional_geo_blocking and >>> >> > routing_name since they don't apply to ALL delivery services and >>> rather >>> >> we >>> >> > enforce those fields in the API >>> >> > 2. we get rid of the ssl_key_version column unless someone knows if >>> it's >>> >> > used and how >>> >> > >>> >> > Thoughts? Concerns? >>> >> > >>> >> > Jeremy >>> >> > >>> >> >>> >> >> >
