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 <alfic...@gmail.com> 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 <alfic...@gmail.com> 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)
