| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-11-05 | |||
| 17:27:34 | lbragstad | you'd just be calling them from two different places | |
| 17:27:58 | johnthetubaguy | lbragstad: yeah totally, I just was wondering if you needed the separation, I think you probably do | |
| 17:28:12 | melwitt | yeah, you're just thinking allow another option where the APIs are decoupled right? and then if they are co-located in some part of code, you could use the context manager | |
| 17:28:27 | lbragstad | ^ yeah | |
| 17:28:33 | melwitt | because enforce == first check and verify == second check with +/- 0 | |
| 17:28:33 | johnthetubaguy | yeah, +1 | |
| 17:28:41 | lbragstad | the context manager could use the same exact public APIs | |
| 17:28:51 | johnthetubaguy | totally agreed with that | |
| 17:28:55 | melwitt | that actually sounds like it fits nicely | |
| 17:29:02 | johnthetubaguy | +1 | |
| 17:29:16 | lbragstad | ok - sounds like we have some things we can improve in oslo.limit then | |
| 17:29:31 | johnthetubaguy | I wondered if you could just have a single check API that is used in both sides of the context manager, but possibly not | |
| 17:29:52 | johnthetubaguy | i.e. use check in enter and exit | |
| 17:30:12 | lbragstad | you mean make enforce() generic enough to handle all cases? | |
| 17:30:16 | johnthetubaguy | but I think I prefer oslo.limit deciding if a recheck is done or not | |
| 17:30:17 | johnthetubaguy | yeah | |
| 17:30:39 | lbragstad | yeah - i think wxy-xiyuan tried to do something like that initially | |
| 17:31:32 | jaypipes | johnthetubaguy: did you want a review on the patch above? | |
| 17:31:52 | jaypipes | johnthetubaguy: I'm not a fan of mixing usage and limit information in the same function/object | |
| 17:32:33 | jaypipes | johnthetubaguy: lemme push up what I've been working on. perhaps it will make a bit more sense. | |
| 17:32:43 | johnthetubaguy | jaypipes: from a direction point of view, that would be great | |
| 17:32:55 | johnthetubaguy | jaypipes: yeah, that would be cool | |
| 17:33:14 | johnthetubaguy | I have to run now, but will catch up later | |
| 17:33:59 | johnthetubaguy | lbragstad: yeah, the clear separation is almost certainly a win over a smaller API | |
| 17:37:41 | melwitt | not to complicate things but thinking about the APIs, I do wonder how the they'll look in the future when other backends are added, like the etcd locking idea as another way to deal with races | |
| 17:38:33 | lbragstad | ^ a couple people also had ideas about using etcd as a way to implement more than two levels of hierarchical checking | |
| 17:40:02 | melwitt | to be clear, just thinking to keep those in mind to avoid painting ourselves into a corner for the future. but decoupling APIs shouldn't affect that really | |
| 17:41:59 | lbragstad | ideally - the usage of etcd would be an implementation detail for oslo.limit to handle | |
| 17:42:28 | lbragstad | i would think - i can't really think of a reason why the data coming from nova would change if etcd were being used | |
| 17:46:47 | melwitt | I think it would too, just thinking where it fits. it would be in enforce right? and then verify wouldn't be used in that case? | |
| 17:47:40 | melwitt | like, is verify() valid depending on backend? | |
| 17:58:43 | efried | bauzas: LazyLoader? | |
| 18:00:10 | bauzas | efried: wat? | |
| 18:00:26 | efried | bauzas: I'm wondering if there's a reason functools.partial was necessary there. | |
| 18:00:41 | bauzas | oh about my concern? | |
| 18:00:53 | bauzas | you mean functools.wraps ? | |
| 18:01:08 | bauzas | .partial is different | |
| 18:01:31 | efried | bauzas: This is unrelated to anything | |
| 18:01:46 | bauzas | not sure I understand you | |
| 18:01:59 | efried | bauzas: I'm running into a unit test snafu due to LazyLoader because I'm trying to access self.compute.reportclient._provider_tree | |
| 18:02:09 | efried | LazyLoader is returning _provider_tree as a functools.partial | |
| 18:02:23 | efried | because it's only set up to return *callable* attributes from the thing it's shimming. | |
| 18:02:54 | efried | So I'm trying to figure out if there's a way for me to do | |
| 18:02:55 | efried | if callable: | |
| 18:02:55 | efried | do the thing it does now | |
| 18:02:55 | efried | else | |
| 18:02:55 | efried | just return the attribute, not a functools.partial | |
| 18:03:13 | efried | but in order to do that, I'm going to have to collapse it | |
| 18:05:03 | efried | bauzas: https://review.openstack.org/#/c/104556/9..14/nova/scheduler/client/__init__.py | |
| 18:05:48 | bauzas | ah this | |
| 18:06:36 | efried | I fully expect you to 100% remember all the reasoning that went on behind this change from four years ago. | |
| 18:07:40 | bauzas | efried: .partial() is because we don't want to pass __name | |
| 18:08:02 | bauzas | example https://stackoverflow.com/questions/15331726/how-does-the-functools-partial-work-in-python | |
| 18:08:44 | efried | bauzas: butbutbut, why wouldn't it work to just do this: | |
| 18:08:44 | efried | def __getattr__(self, name): | |
| 18:08:44 | efried | if self.instance is None: | |
| 18:08:44 | efried | self.instance = self.klass(*self.args, **self.kwargs) | |
| 18:08:44 | efried | return getattr(self.instance, name) | |
| 18:10:47 | efried | (FWIW, it seems to do what I need) | |
| 18:12:07 | bauzas | yup, but then that's not a lazy loader ;) | |
| 18:12:23 | efried | bauzas: Say wha? | |
| 18:13:23 | efried | bauzas: The only difference in laziness is if a caller pulled a method but didn't call it. | |
| 18:13:32 | bauzas | efried: calling __getattr__ in your case will run getattr() of the instance, right? | |
| 18:13:47 | efried | yes | |
| 18:14:07 | bauzas | so, it's executing synchronously | |
| 18:14:28 | bauzas | the problem is when importing the module | |
| 18:14:46 | bauzas | if you don't lazy load it, then you're loopîng | |
| 18:15:09 | bauzas | see why I lazy-load it below | |
| 18:15:14 | efried | You mean getattr looping on itself? | |
| 18:15:25 | bauzas | nope | |
| 18:15:52 | bauzas | we will only import the modules when we call select_destinations() | |
| 18:16:14 | bauzas | *not when we import nova.scheduler.client* :) | |
| 18:16:40 | efried | that... still happens? | |
| 18:17:15 | efried | Never mind about why it matters that we wait to import the report client... | |
| 18:17:19 | bauzas | efried: see this blogpost https://snarky.ca/lazy-importing-in-python-3-7/ | |
| 18:19:08 | bauzas | efried: anyway, test it | |
| 18:19:22 | bauzas | efried: when I wrote this, we were having an import loop | |
| 18:19:29 | bauzas | hence the lazyload | |
| 18:19:32 | efried | bauzas: Test it how? Is there test code somewhere that verifies that the importing is being done lazily? | |
| 18:19:43 | bauzas | but now we changed a lot of the client, so maybe it's not needed | |
| 18:20:01 | bauzas | efried: you can pdb it, right? | |
| 18:20:11 | efried | I would think so. | |
| 18:20:19 | bauzas | or you can just remove the lazyloader and test whether we still have the import loop | |
| 18:20:30 | bauzas | if you have concerns by this class | |
| 18:20:39 | bauzas | anyway, 7:20pm here | |
| 18:20:40 | bauzas | ++ | |
| 18:20:54 | efried | def __init__(self): | |
| 18:20:54 | efried | self.queryclient = LazyLoader(importutils.import_class( | |
| 18:20:54 | efried | 'nova.scheduler.client.query.SchedulerQueryClient')) | |
| 18:20:54 | efried | self.reportclient = LazyLoader(importutils.import_class( | |
| 18:20:54 | efried | 'nova.scheduler.client.report.SchedulerReportClient')) | |
| 18:21:07 | efried | Pretty sure that -^ is actually doing the import right away. | |
| 18:21:47 | efried | You're calling LazyLoader with the *result* of running importutils.import_class(), which is the imported class. | |
| 18:24:06 | lbragstad | melwitt that's a good question, verify is going to need the current usage and limit information, which means it might need to use etcd | |
| 18:24:10 | lbragstad | to get that information | |
| 18:24:18 | cdent | efried: sure looks that way to me too | |
| 18:24:27 | cdent | the reason it avod the import loop is simply because it is called "later" | |
| 18:24:34 | cdent | but it isn't lazy | |
| 18:24:43 | efried | yuh. I guess I need to pull this part out into a separate patch, sigh. | |
| 18:26:11 | melwitt | lbragstad: I was wondering, is verify() even useful though if we are distributed locking on enforce + create? I dunno, just unhelpfully thinking aloud :P | |