+1 on listing them as an array. JSON arrays are defined to be ordered, so
that lets us use the array order as the lookup order, making it easy to add
that as a feature in the future.
-1 on array types in the database. It's unnecessary denormalization, and
will be harder to deal with in the future. We can do the same thing with a
normal join table:
cg | localization | priority
----------------------------
foo | coverage_zone | 1
foo | maxmind | 2
foo | deep | 0
bar | coverage_zone | 0
Which maps to the config `"permissions": [ "deep", "coverage-zone",
"geo"],` and also addresses your points on future types (we just add a new
enum value to the localization type). The additional `priority` column lets
us order it in the JSON array via SELECT ORDER BY.
+1 on rethinking the cachegroup fallback API, too. We should have more join
tables, and better data normalization, but it's painful to use HTTP/JSON
APIs like that. Our APIs will be much easier to use, if we can stop
exposing join tables to the API altogether (e.g. cachegroup_fallback,
deliveryservice_regex), and only insert them into their proper objects
(cachegroup, deliveryservice), and then manipulate them within the object
via POST/PUT/PATCH on the larger object.
On Thu, Jul 5, 2018 at 9:23 AM, Mark Torluemke <mtorlue...@apache.org>
wrote:
> Inline.
>
> On Thu, Jul 5, 2018 at 9:00 AM, Rawlin Peters <rawlin.pet...@gmail.com>
> wrote:
>
> > You're not mistaken, Jonathan, that is how the logic basically flows
> > today; however, with cachegroup fallback configuration now it can be
> > deep[if enabled] -> cz -> geo[if fallback config allows].
> >
> > Marking a cachegroup as "CZ-only" is really about filtering possible
> > cachegroups in selection *after* a client's location is discovered (by
> > geoIP DB). So it can really be described as "given the method of
> > client localization, filter out certain cachegroup targets".
> >
> > "Deep-only": filtered out after the "CZ localization" and "post-Geo
> > localization" steps
> > "CZ-only": filtered out after the "Geo localization" step
> > "Geo-only": filtered out after the "deep/regular-CZ" steps
> >
> > Right now, those all seem somewhat mutually exclusive (except CZ kind
> > of includes both deep and regular CZ), unless you wanted to do
> > something like "Deep and Geo only". I don't know if that's a valid use
> > case, but maybe "CZ and Future-thing only" would make sense. So maybe
> > cachegroups should really be described as:
> >
> > "Deep-enabled": allowed after "deep-CZ localization" step
> > "CZ-enabled": allowed after the "regular-CZ localization" step
> > "Geo-enabled": allowed after the "geo localization" step
> > "Future-thing-enabled": allowed after the "future-thing" step
> >
>
> +1. Well said.
>
>
> >
> > Then we could add multiple permissions to a cachegroup rather than
> > making them mutually exclusive. For the initial use case I proposed
> > this for, I would just be marking the cachegroups that aren't directly
> > connected to the internet as "Deep-enabled, CZ-enabled,
> > future-thing-enabled" rather than "CZ-only". "Deep-only" cachegroups
> > would be marked "deep-enabled".
> >
> > If no permissions are configured, all permissions are given. Enabling
> > one permission means all other permissions are disabled.
> >
> > I think this could be done by adding an array-type column to the
> > cachegroup table/API like so:
> >
> > {
> > "name": "cache_group_edge",
> > "shortName": "cg_edge",
> > "latitude": 12,
> > "longitude": 45,
> > "permissions": [
> > "DEEP",
> > "CZ",
> > "GEO"
> > ],
> > "typeId": 6
> > }
> >
>
> The array feels a little fragile, perhaps. Should we decorate each entry
> with an "order", for completeness? (It's obvious to us that the order is
> DEEP->CZ->GEO, but might not be for new users.)
>
>
> > The "permissions" field would remain optional, meaning that no
> > permissions chosen assumes *all* permissions are enabled (giving you
> > the existing default behavior). Postgres has array-type columns which
> > would map to this API quite nicely. In fact, I'm also kind of
> > rethinking the "Cachegroup Fallback" API after going over this,
> > because listing a cachegroup's fallbacks in an array like this could
> > make sense and remove the need for the new cachegroup_fallbacks API...
>
>
> +1 here as well (without knowing the ins-and-outs of the Cachegroup
> Fallback API).
>
>
>
> > On Wed, Jul 4, 2018 at 3:47 PM Gray, Jonathan <jonathan_g...@comcast.com
> >
> > wrote:
> > >
> > > If I'm not mistaken, today the logic is fall through on miss (deep[if
> > enabled] -> cz -> geo). The suggestion to make a 1:* relationship moves
> > that from being implicit in the code to explicit choice by the DS owner.
> > Changing the database/apis is expensive enough that maybe we can't make
> > that jump near term. If you do only add a single field for this using an
> > enum, there should be a default 'fallthrough' or 'native' option to
> > replicate the old behavior. Also, I misspoke earlier, this isn't about
> the
> > routing methods it's about localization methods.
> > >
> > > On 7/4/18, 11:18 AM, "Jeremy Mitchell" <mitchell...@gmail.com> wrote:
> > >
> > > Can a cache group have many "routing methods". If not, would one
> > column
> > > (cachegroup.routing_method) suffice where acceptable values are an
> > enum? (
> > > CZ, DEEP, GEO)
> > >
> > > On Tue, Jul 3, 2018 at 7:05 PM, Mark Torluemke <
> > mtorlue...@apache.org>
> > > wrote:
> > >
> > > > Feels like columns in the cachegroup table could go a long way in
> > the short
> > > > term -- routing_cz, routing_geo, routing_deep?
> > > >
> > > > On Tue, Jul 3, 2018 at 1:04 PM, Rawlin Peters <
> > rawlin.pet...@gmail.com>
> > > > wrote:
> > > >
> > > > > Thanks for the suggestion, Jonathan. I wasn't even thinking
> > about the
> > > > > possibility of "deep-only" cachegroups, but I could definitely
> > see
> > > > > that as a future use case.
> > > > >
> > > > > Do you envision something like a table of cachegroup
> permissions
> > > > > (cachegroup_id, cachegroup_permission enum type), where the
> enums
> > > > > would be DEEP_ONLY and CZ_ONLY to start with? That would
> > definitely
> > > > > increase initial dev time a bit and might require some new API
> > > > > endpoints to add/remove permissions unless we can make the new
> > API
> > > > > field use an array value.
> > > > > On Tue, Jul 3, 2018 at 12:23 PM Gray, Jonathan
> > > > > <jonathan_g...@comcast.com> wrote:
> > > > > >
> > > > > > Rather than a cz_only flag, it might be more powerful to have
> > a join
> > > > > table with allowed routing methods. You could cover the same
> > use case,
> > > > but
> > > > > it would also allow you to do certain things like deep_only or
> > > > effectively
> > > > > not-geo.
> > > > > >
> > > > > > - Jonathan
> > > > > >
> > > > > > On 7/3/18, 12:03 PM, "Rawlin Peters" <
> rawlin.pet...@gmail.com>
> > wrote:
> > > > > >
> > > > > > Hey Traffic Controllers,
> > > > > >
> > > > > > I'd like to be able to mark a cachegroup as "CZ-only" via
> > the API
> > > > so
> > > > > > that off-net clients (i.e. IPs that aren't included in
> the
> > Coverage
> > > > > > Zone File) don't get routed to "CZ-only" cachegroups.
> This
> > is
> > > > because
> > > > > > some cachegroups aren't directly connected to the
> internet
> > and
> > > > > routing
> > > > > > off-net clients to those cachegroups is expensive. This
> > proposal
> > > > > would
> > > > > > change Traffic Router behavior to route these clients to
> > the
> > > > closest
> > > > > > available cachegroup that's NOT marked as "CZ-only".
> > > > > >
> > > > > > My proposal is to add a boolean column to the cachegroup
> > table -
> > > > > > cz_only - which is then included in the CRConfig's
> > "edgeLocations"
> > > > > for
> > > > > > Traffic Router to parse and look up when routing off-net
> > clients.
> > > > By
> > > > > > default it will be false to keep the existing behavior.
> > The new
> > > > field
> > > > > > will be optional in the API but NOT NULL in the DB, so
> > null values
> > > > in
> > > > > > requests will explicitly be set to false in the DB.
> > > > > >
> > > > > > Questions/concerns?
> > > > > >
> > > > > > - Rawlin
> > > > > >
> > > > > >
> > > > >
> > > >
> > >
> > >
> >
>
