Here is the PR for the initial version of the code changes discussed for https://github.com/apache/incubator-trafficcontrol/issues/1907
https://github.com/apache/incubator-trafficcontrol/pull/2029 Did minimal testing. Hosting it before testing it fully to get early review comments. Thanks, Vijayanand S On Tue, Mar 20, 2018 at 9:51 PM, Rawlin Peters <[email protected]> wrote: > I think we should do a few things: > 1. POST: receive a full list of ordered backups and create the > fallback list. If the fallback list already exists, render a failure. > 2. PUT: receive a full list of ordered backups, delete the existing > fallback list, and create a new fallback list. If the fallback list > does not exist yet, render a failure. > 3. DELETE: delete the existing fallback list for a cachegroup. If the > fallback list does not exist, render a failure. > 4. GET: return the existing fallback list for a cachegroup. If the > fallback list does not exist, render a failure. > > Then the client behavior would be: > GET the current fallbacks for a cachegroup. If the list exists, make > updates and PUT the entire updated list. If the list does not exist, > create a list and POST the entire list. To empty the list, use the > DELETE endpoint (or do a PUT with an empty list). > > Does that sound good? Right now it doesn't make sense for a client to > do a PUT for just a single fallback entry, unless we add some sort of > insert/append/prepend PUT APIs which would seem superfluous. > > - Rawlin > > > > On Mon, Mar 19, 2018 at 11:29 PM, Vijay Anand > <[email protected]> wrote: > > Shall i take this format :[{"name": "grp1", "order": 1},{"name": "grp3", > > "order: 2}] > > > > @Rawlin > >>Perhaps the POST/PUT endpoints are basically the same and always take the > > full list of backups. > > > > Wanted to make sure the behavior of PUT/ POST endpoints; POST will delete > > an existing set of backups and insert the new ones. While PUT will update > > them. > > > > For example, I have grp2(1) and grp3(2) as backups for grp1: > > > > Case #1: I get a PUT with grp2 (3), I will update grp2 with new order 3. > > grp1 still has two backup cache groups. > > > > Case #2: I get a POST with gr2(3), I will delete all the existing entries > > and insert grp2 making grp2 as the only backup for grp1 with order 3. > > > > Isnt that right? > > > > -Vijayanand S > > > > On Tue, Mar 20, 2018 at 8:06 AM, Chris Lemmons <[email protected]> > wrote: > > > >> You can keep each object without the order parameter: > >> > >> [{"name": "grp1"}, {"name":"grp3"}] > >> > >> Nevertheless, it sounds like I'm a minority opinion on this one. It's > >> not that important of an issue, I think. Either way will work. > >> > >> On Mon, Mar 19, 2018 at 5:50 PM, Rawlin Peters <[email protected] > > > >> wrote: > >> > Yeah, maybe it doesn't make sense to update just one backup entry at a > >> > time because you'd have to update the order of the rest of the backups > >> > as well. Perhaps the POST/PUT endpoints are basically the same and > >> > always take the full list of backups. But I think we should still keep > >> > each entry as an object with an explicit "order" so that we can extend > >> > it more easily in the future. I could easily see us adding a "weight" > >> > to each entry, so that a particular cachegroup could split the > >> > fallback traffic between multiple cachegroups at a time rather than > >> > falling back through them sequentially. > >> > > >> > - Rawlin > >> > > >> > On Mon, Mar 19, 2018 at 2:30 PM, Chris Lemmons <[email protected]> > >> wrote: > >> >> So, to continue the conversation, it looks like the list of backup > >> >> groups is stored as a List<String>. It's currently loaded by > iterating > >> >> the elements of the json array in order. That looks great to me. > >> >> > >> >> It seems odd to me to have a separate order parameter in the API. > >> >> Since order has to be unique, it's unlikely that you'd be able to > >> >> update the order component to do anything other than "move to an end" > >> >> without updating about half the rows in the db anyway. It just feels > >> >> like we're asking more of the API user. > >> >> > >> >> On Mon, Mar 19, 2018 at 2:04 PM, Chris Lemmons <[email protected]> > >> wrote: > >> >>> Moving a conversation started in Slack to the mailing list: > >> >>> > >> >>> VijayAnand [8:06 AM] > >> >>> Hi > >> >>> > >> >>> This is regarding TR's Backup Cache Group Selection > >> >>> which is https://github.com/apache/incubator-trafficcontrol/ > >> issues/1907 > >> >>> GitHub > >> >>> Deterministic Cachegroup failover · Issue #1907 · > >> >>> apache/incubator-trafficcontrol > >> >>> Currently, if all caches in a cache group are unavailable Traffic > >> >>> Router will route clients to the next closest cache group. This > works > >> >>> great for some networks but could cause problems in others. T... > >> >>> Based on Rawlin's review comments on the PR > >> >>> https://github.com/apache/incubator-trafficcontrol/pull/1908 > >> >>> GitHub > >> >>> Changes for Backup Edge Cache Group by Vijay-1 · Pull Request #1908 > · > >> >>> apache/incubator-trafficcontrol > >> >>> This PR implements solution for the issue: #1907 It places the > backup > >> >>> policy in the CZF file { "coverageZones": { > >> >>> "GROUP2": { "backupList": ["GROUP1"], > >> >>> &quo... > >> >>> I have the following Schema to support this which is in-sync with > >> >>> Rawlin's comments > >> >>> CREATE TABLE cachegroup_fallbacks ( > >> >>> primary_cg bigint, > >> >>> backup_cg bigint CHECK (primary_cg != backup_cg), > >> >>> set_order bigint DEFAULT 0, > >> >>> CONSTRAINT fk_primary_cg FOREIGN KEY (primary_cg) REFERENCES > >> >>> cachegroup(id) ON DELETE CASCADE, > >> >>> CONSTRAINT fk_backup_cg FOREIGN KEY (backup_cg) REFERENCES > >> >>> cachegroup(id) ON DELETE CASCADE, > >> >>> UNIQUE (primary_cg, backup_cg), > >> >>> UNIQUE (primary_cg, set_order) > >> >>> ); > >> >>> > >> >>> ALTER TABLE cachegroup ADD COLUMN fallback_to_closest BOOLEAN > DEFAULT > >> TRUE; > >> >>> Would like to get your views before i start coding for the same > >> >>> > >> >>> Eric Friedrich [8:15 AM] > >> >>> why does the set_order get a default? > >> >>> > >> >>> VijayAnand [8:17 AM] > >> >>> aah. that is not needed > >> >>> assuming there is a valid input > >> >>> for order > >> >>> > >> >>> Rawlin Peters [9:15 AM] > >> >>> yeah a null value there could be interpreted as zero in the API, but > >> >>> maybe we should add a NOT NULL constraint to the columns as well? I > >> >>> can't think of any reasons why any column should be optional > >> >>> > >> >>> Eric Friedrich [9:18 AM] > >> >>> any preference in the API between > >> >>> ```{"list": ["grp1", "grp2"]}``` > >> >>> vs > >> >>> ```{"list": [{"name": "grp1", "order": 1}, > >> >>> {"name": "grp3", "order: 2}, // or 5 or some > other > >> >>> positive integer > >> >>> ] > >> >>> }``` > >> >>> > >> >>> Rawlin Peters [9:36 AM] > >> >>> 2nd option would make it easier to extend in the future with > weighting > >> >>> for instance, we could just add another key/value in that object > >> >>> > >> >>> Eric Friedrich [9:37 AM] > >> >>> good point- i hadn’t thought about that > >> >>> > >> >>> Chris Lemmons [1:16 PM] > >> >>> The second is way harder to parse and handle. I strongly prefer the > >> first. > >> >>> Is there a reason to expect such extension? (edited) > >> >>> > >> >>> Jeff Elsloo [1:30 PM] > >> >>> presumably we’re using a library to handle it, so why is it way > harder? > >> >>> > >> >>> Dan Kirkwood [1:33 PM] > >> >>> +1 -- it's all just JSON.. > >> >>> > >> >>> Rob Butts [1:39 PM] > >> >>> I don’t agree that we shouldn’t care about the human-readability, > >> >>> because we have libraries. Someone might want to write an app in a > >> >>> different language. I wouldn’t say “way” harder, but the second > >> >>> definitely is harder to read, and unintuitive. I’m +1 on making our > >> >>> APIs easy to read and work with natively :confused: > >> >>> > >> >>> Jeff Elsloo [1:41 PM] > >> >>> but a list is just a container > >> >>> it contains “things” > >> >>> in this case it’s a list of objects with properties > >> >>> > >> >>> Dylan Volz [1:41 PM] > >> >>> agree we shouldn't not care, but most of our apis are lists of json > >> >>> objects, and if we need to add a key the the second definition is > far > >> >>> more easily extensible > >> >>> > >> >>> Chris Lemmons [1:42 PM] > >> >>> True. Are they actually objects with properties? Or are they string > >> values? > >> >>> > >> >>> Jeff Elsloo [1:42 PM] > >> >>> that’s implied by the structure > >> >>> > >> >>> Chris Lemmons [1:42 PM] > >> >>> Either way, I'd prefer not to overload the order as a parameter, > >> >>> unless there is a particular reason? > >> >>> > >> >>> Jeff Elsloo [1:43 PM] > >> >>> `{"name": "grp3", "order: 2}` is not a string in JSON > >> >>> > >> >>> Chris Lemmons [1:43 PM] > >> >>> Aye. > >> >>> > >> >>> Jeff Elsloo [1:43 PM] > >> >>> I don’t follow what you mean about order > >> >>> > >> >>> Chris Lemmons [1:43 PM] > >> >>> Are there other parameters (or might there reasonably be in the > >> future)? > >> >>> > >> >>> Jeff Elsloo [1:43 PM] > >> >>> order is just a property and has nothing to do with overloading > >> anything > >> >>> in this context, if it has to do with Rawlin’s steering work, yes > >> >>> there certainly could be other properties > >> >>> which is why the format was suggested > >> >>> > >> >>> Chris Lemmons [1:45 PM] > >> >>> So, for example, using objects: > >> >>> > >> >>> I prefer `[{"name": "grp1"}, {"name": "grp3"}]` to `[{"name": > "grp1", > >> >>> "order": 1},{"name": "grp3", "order: 2}]`. > >> >>> In both cases, you can easily add a new parameter, for example, if > >> >>> groups had an owner. > >> >>> But arrays already have orders. > >> >>> > >> >>> Jeff Elsloo [1:45 PM] > >> >>> no > >> >>> not in JSON > >> >>> > >> >>> Chris Lemmons [1:46 PM] > >> >>> Yes, they do. > >> >>> > >> >>> Jeff Elsloo [1:46 PM] > >> >>> I’m not going to implicitly trust the order of what’s in the source > >> JSON > >> >>> we don’t have the ability to control that > >> >>> > >> >>> Chris Lemmons [1:46 PM] > >> >>> Arrays in JSON are ordered. Objects in JSON are not ordered. > >> >>> > >> >>> Jeff Elsloo [1:46 PM] > >> >>> no they aren’t > >> >>> not unless they’re systematically created that way > >> >>> which in this case they are not > >> >>> we have a property called order and a value > >> >>> that could appear in any order of the underlying array > >> >>> we would have to do a lot more work to build it properly and then > just > >> >>> make the assumption in downstream components that the order is > >> >>> implicitly correct in JSON > >> >>> that is a risky assumption > >> >>> > >> >>> Chris Lemmons [1:47 PM] > >> >>> https://tools.ietf.org/html/rfc7159 > >> >>> > >> >>>> An array is an ordered sequence of zero or more values. > >> >>> > >> >>> Jeff Elsloo [1:47 PM] > >> >>> seriously Chris? > >> >>> do you even know why or how such things are used in Traffic Ops? > >> >>> did you know that the orders could be negative? > >> >>> I’m not arguing about whether or not JSON supports order in its > arrays > >> >>> I’m arguing about how the data is put into JSON and how users > >> >>> configure the properties > >> >>> and what the values can be > >> >>> > >> >>> Chris Lemmons [1:49 PM] > >> >>>> do you even know why or how such things are used in Traffic Ops? > >> >>> > >> >>> Not in detail, no. Hence my question: > >> >>> > >> >>>> unless there is a particular reason? > >> >>> > >> >>> Jeff Elsloo [1:49 PM] > >> >>> having to order that in the JSON adds another layer of complexity > that > >> >>> is unnecessary > >> >>> I’ve given you some but you dropped an RFC on me in reply which is > >> >>> implicitly saying you don’t agree and won’t > >> >>> so for cache group ordering, which is separate from the steering > >> >>> ordering issue, I think that stating order specifically will give us > >> >>> more options down the road just as it did with steering > >> >>> we have multiple ways to order things in the steering case, and > we’ve > >> >>> extended that, so I don’t see why that precedent wouldn’t also apply > >> >>> here > >> >>> backup cache group ordering that is > >> >>> > >> >>> Chris Lemmons [1:55 PM] > >> >>> Ah... I think there was a bit of a miscommunication there. To the > >> >>> point of the discussion, though... I missed which data this is > >> >>> storing. Yeah, we have to store the ordering data explicitly in the > >> >>> database, since rows are unordered. > >> >>> > >> >>> Jeff Elsloo [1:55 PM] > >> >>> yes, that’s what I was getting at.. so we’d have to pull the values > >> >>> out of the database, sort them properly, then ensure they are > written > >> >>> that way to the snapshot table > >> >>> > >> >>> Rawlin Peters [1:55 PM] > >> >>> fwiw the particular JSON in question isn't for the steering stuff > I'm > >> >>> working on, it's for the Cachegroup Failover stuff (i.e. CG1 > >> >>> specifically fails over to CG2, CG3, in that order) > >> >>> > >> >>> Jeff Elsloo [1:55 PM] > >> >>> it’s easier to just have the downstream consumer worry about that, > as > >> >>> they likely would need to worry about it anyway > >> >>> yeah Rawlin, I saw that eventually :slightly_smiling_face: > >> >>> > >> >>> Rawlin Peters [1:56 PM] > >> >>> sorry just catching up too > >> >>> > >> >>> Jeff Elsloo [1:56 PM] > >> >>> but to me it’s a similar concept as steering > >> >>> we have an ordering mechanism based on an int value today > >> >>> maybe that changes in the future > >> >>> as things in our world tend to > >> >>> I agree with keeping the content the API spits out as simple as > >> >>> possible, but not to the extent that it makes us have to do more > work > >> >>> just for sake of readability > >> >>> > >> >>> Chris Lemmons [1:58 PM] > >> >>> So, what happens if ordering is equivalent? > >> >>> > >> >>> Rawlin Peters [1:58 PM] > >> >>> yeah plus we need a way to update the order of a particular > relation, > >> >>> which is why we can't just keep it as a simple list of strings > >> >>> there's a uniqueness constraint on (primary_cg, order), so a > >> >>> particular cachegroup can only declare one backup CG at that > >> >>> particular ordering > >> >>> > >> >>> Chris Lemmons [2:01 PM] > >> >>> Ok. So if you want to change ordering other than "move to end" or > >> >>> "move to beginning", you'll probably need to update half of the > >> >>> relations? > >> >>> > >> >>> Dave Neuman [2:01 PM] > >> >>> All of this ^^ should be done on the mailing list (edited) > >> >>> any of the decisions made here don't count > >> >>> unless you want to come to some consensus here, put it on the > mailing > >> >>> list, and have everyone +1 > >> >>> which is against the spirit of the mailing list (edited) > >> >
