| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-06-27 | |||
| 14:41:20 | jaypipes | edleafe: ^ | |
| 14:43:36 | edleafe | jaypipes: one of your stated concerns was the chance scheduler. Chance scheduler doesn't use HostState. It's specific to filter scheduler | |
| 14:44:05 | edleafe | jaypipes: so I'm not sure how this is separating things more cleanly | |
| 14:44:08 | jaypipes | edleafe: right, which is why I changed the returned value from select_destinations() to be a list of dicts that included the UUID. | |
| 14:45:05 | edleafe | 09:40 < jaypipes>| or even if we did return a list of host state objects, I actually wouldn't care, as long as the manager isn't modifying those objects. | |
| 14:45:10 | jaypipes | edleafe: because the manager will loop through the allocation requests and attempt to claim resources on the destinations returned by select_destinations(). If select_destinations() doesn't return something with that UUID, the manager doesn't have the ability to match an allocation request to a host. | |
| 14:46:19 | edleafe | jaypipes: well, if keeping claiming out of the filter scheduler is worth it, then go for it | |
| 14:46:21 | jaypipes | edleafe: the alternative is to have the _schedule() method of the filter scheduler driver do everything under the sun: filter, weigh, get allocation requests, and claim. | |
| 14:47:10 | edleafe | jaypipes: and IYO it's *so* much cleaner to have the _schedule method do filter, weigh, and get allocation requests | |
| 14:47:34 | jaypipes | edleafe: no, I have the _schedule() method only do filter and weigh. | |
| 14:47:44 | jaypipes | edleafe: the manager does the claiming. | |
| 14:47:47 | edleafe | jaypipes: In my mind, the claim is a final filter. _schedule() isn't doing its job if it returns an invalid host | |
| 14:47:52 | jaypipes | after receiving the set of destinations from the driver. | |
| 14:48:20 | jaypipes | edleafe: what do you mean by "returns an invalid host"? I lost you on that one. | |
| 14:48:29 | jaypipes | and I don't see the claim as a final filter at all. | |
| 14:48:34 | jaypipes | it's a completely different operation. | |
| 14:48:37 | dansmith | definitely not | |
| 14:48:42 | dansmith | if you mean a filterscheduler filter | |
| 14:48:46 | edleafe | jaypipes: in the case of horizontal scaling of the scheduler, when there could be races for the same host | |
| 14:49:11 | edleafe | jaypipes: a filter is logic that determines if a host is acceptable. | |
| 14:49:12 | dansmith | how does claiming in a filter make that better? | |
| 14:49:23 | edleafe | dansmith: not "in a filter". | |
| 14:49:28 | jaypipes | edleafe: there definitely *will* be races for the same host. And the manager will be the thing responsible for retrying those claim attempts on the same or an alternate matching host. | |
| 14:49:37 | dansmith | jaypipes: right | |
| 14:49:39 | edleafe | dansmith: it's the final step in the process | |
| 14:49:47 | dansmith | edleafe: in the manager. | |
| 14:50:00 | edleafe | ok, fine, fine | |
| 14:50:26 | edleafe | it just looks to me like you're trading one entanglement for another.. | |
| 14:50:32 | edleafe | But don't let that slow you down | |
| 14:50:44 | jaypipes | .. | |
| 14:50:51 | dansmith | I don't even know how to respond to that | |
| 14:50:58 | jaypipes | I'm not going to. | |
| 14:51:02 | edleafe | you don't have to | |
| 14:51:08 | edleafe | I've stated my concerns | |
| 14:51:20 | jaypipes | I'll go back to trying to clean up the proposed code and add commentary to it. | |
| 14:51:30 | dansmith | jaypipes: before you do, we need to have a serious talk | |
| 14:51:37 | edleafe | if you still want to go down that route, I'll accept that | |
| 14:51:53 | jaypipes | dansmith: yes? | |
| 14:52:09 | dansmith | jaypipes: a couple months ago you started doing this trailing paren-on-a-line-by-itself thing, which I argued about, but let you continue for some large function calls in tests | |
| 14:52:22 | dansmith | jaypipes: but this patch has them all over the place and for calls with only a few parameters that could be a couple lines | |
| 14:52:35 | dansmith | jaypipes: _none_ of our other code looks like that and it's an abrupt change recently | |
| 14:52:50 | dansmith | it looks very different to everything else and it really (really) bugs me | |
| 14:53:28 | jaypipes | dansmith: k, I'll restyle | |
| 14:53:43 | dansmith | jaypipes: I'd *really* appreciate it :) | |
| 14:54:58 | jaypipes | dansmith: just for the parens, right? not the dict notation? | |
| 14:55:21 | dansmith | jaypipes: the parens are what make my eyes bleed, but give me a dict notation example | |
| 14:55:45 | dansmith | if you mean just ending on a line with } that's fine | |
| 14:55:54 | jaypipes | yeah | |
| 14:56:10 | jaypipes | k, will change. | |
| 14:56:17 | dansmith | yeah, we do that everywhere and that's cool | |
| 14:56:39 | jaypipes | dansmith: no prob, will fix. | |
| 14:56:45 | jaypipes | dansmith: just for the record, I find this: | |
| 14:56:47 | jaypipes | resource_query = ",".join( | |
| 14:56:47 | jaypipes | sorted("%s:%s" % (rc, amount) | |
| 14:56:47 | jaypipes | for (rc, amount) in resources.items()) | |
| 14:56:47 | jaypipes | ) | |
| 14:56:55 | dansmith | le barf | |
| 14:56:59 | jaypipes | easier to read than this: | |
| 14:56:59 | jaypipes | resource_query = ",".join(sorted("%s:%s" % (rc, amount) | |
| 14:57:00 | jaypipes | for (rc, amount) in resources.items())) | |
| 14:57:10 | dansmith | you can make it three lines, | |
| 14:57:19 | dansmith | just don't put the trailing paren on a line by itself | |
| 14:57:30 | jaypipes | loud and clear sir. | |
| 14:57:34 | dansmith | but even that is slightly less smelly than: | |
| 14:57:37 | dansmith | myfunc( | |
| 14:57:40 | dansmith | foo, bar, | |
| 14:57:41 | dansmith | ) | |
| 14:57:45 | dansmith | le double barf ^ | |
| 14:57:49 | jaypipes | heh | |
| 15:03:56 | openstackgerrit | Andreas Scheuring proposed openstack/nova master: DO NOT MERGE! Dummy patch https://review.openstack.org/478186 | |
| 15:08:31 | efried | dansmith +1, thanks for bringing a hammer to that party. | |
| 15:09:09 | dansmith | efried: interventions are sometimes necessary to correct misbehavior | |
| 15:09:48 | dansmith | I learned that from TLC | |
| 15:09:54 | efried | The more you know | |
| 15:10:15 | dansmith | the L used to stand for learning, but now it stands for something else like "look-at-the-freaks" | |
| 15:11:34 | jaypipes | dansmith: https://review.openstack.org/#/c/475448/13/nova/tests/functional/api/openstack/placement/fixtures.py | |
| 15:11:35 | efried | Never had the cable, so I'll have to take your word for it. (I have absolutely no trouble wasting enough time with Netflix and Amazon Prime.) | |
| 15:11:56 | jaypipes | dansmith: do you want me to de-parens those fixutres like on line 262? | |
| 15:13:09 | dansmith | jaypipes: yes, gross | |
| 15:13:30 | jaypipes | dansmith: ok. | |
| 15:13:55 | jaypipes | dansmith: so it's really just the lone paren on a line that you don't like, right? | |
| 15:14:25 | dansmith | jaypipes: and the trailing comma in a call that goes with it | |
| 15:14:37 | efried | I'll restate dislike for the trailing comma, which loses its benefit (two-line change becomes one-line change) if you pull the paren back. | |
| 15:14:40 | efried | yeah, what he said. | |
| 15:15:07 | dansmith | jaypipes: I don't really see the need to break every call into a multiline one-per-line verbose list either, but.. that's less smelly than the lone paren and trailing comma thing | |
| 15:15:14 | dansmith | for long ones, or ones with a very long parameter name, sure | |
| 15:15:30 | jaypipes | different style, is all. | |
| 15:15:37 | jaypipes | but I'll work with you. | |
| 15:15:49 | dansmith | jaypipes: I know, and we do some of that in other code.. we do not, however, do the trailing comma and lone paren thing :) | |
| 15:16:03 | openstackgerrit | Eric Fried proposed openstack/nova master: Add formatting to scheduling activity diagram https://review.openstack.org/476204 | |
| 15:16:55 | jaypipes | dansmith, efried: you guys would hate Golang then :) | |
| 15:16:57 | efried | I personally dig the one-per-line thing for really long param lists. I'll take the vspace hit for readability there. | |
| 15:17:18 | dansmith | jaypipes: it's consistency with the rest of our code that I care about | |
| 15:17:24 | jaypipes | understood. | |
| 15:17:29 | dansmith | jaypipes: if all our code looked like that it would be fine (ugly, but fine) | |
| 15:17:50 | dansmith | efried: for long param lists, I do it myself sometimes, yeah | |
| 15:25:35 | jaypipes | dansmith: I'\ | |
| 15:25:35 | jaypipes | m goin\ | |
| 15:25:35 | jaypipes | g to start codi\ | |
| 15:25:35 | jaypipes | ng like this just fo\ | |