Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2022-06-22
12:22:10 bauzas ie. 15:00 for CEST and 14:00 for BSR
12:22:10 sean-k-mooney im in a meeting till the top of the hour
12:22:15 bauzas BSTR*
12:22:19 bauzas graaah
12:22:21 bauzas BST*
12:22:24 sean-k-mooney and then i have a dental apointmnt in an hour
12:22:43 sean-k-mooney so it would have to be later today or you and gibi can proceed with Uggla without me
12:22:43 bauzas ok, then let's discuss about Uggla's spec later then
12:22:51 gibi later then
12:23:07 gibi but as far as I see sean-k-mooney stated his oppinion above and I agreed
12:24:16 Uggla bauzas, sean-k-mooney bauzas, I better understand sean-k-mooney proposal that sounds good.
12:25:31 bauzas kk
12:25:37 sean-k-mooney i just pushed my comments on the sepc too
13:09:46 Uggla bauzas, gibi if you can have a look at my latest comments on https://review.opendev.org/c/openstack/nova/+/831507. On 2 points your opinions diverge and I can make both of you pleased at the same time. So I need a consensus between both of you to implement the solution you agree on.
13:13:50 bauzas Uggla: sure, will look
13:24:26 opendevreview Amit Uniyal proposed openstack/nova master: Adds validation for hw machine type in host caps https://review.opendev.org/c/openstack/nova/+/847126
13:26:45 gibi Uggla: responded, fine with bauzas' suggestions
13:31:26 bauzas gibi: Uggla: replied too
13:33:25 bauzas Uggla: we already this kind of sentinel for example in https://github.com/openstack/nova/blob/master/nova/scheduler/manager.py#L114
13:34:15 bauzas that's how we flag an unset param
13:34:59 bauzas this is a simple pattern
13:38:50 gibi I think the special case here is that the caller needs differentiate too if some AZ value is needed to pass forward or the param should not be provided
13:39:58 bauzas gibi: not sure I understand
13:40:09 bauzas gibi: either the AZ is passed or not
13:40:15 bauzas but the value can be None
13:40:16 bauzas right?
13:40:38 gibi by making the new_az optional the caller either needs to pass a value (including None) or not pass the parameter at all
13:40:58 gibi that will be either a conditional on the caller
13:41:17 bauzas https://specs.openstack.org/openstack/nova-specs/specs/zed/approved/unshelve-to-host.html#proposed-change
13:41:19 gibi or the caller could pass the sentinel and avoif the conditional
13:41:35 bauzas ah I see your concern
13:41:47 bauzas well, in general we do this kind of conditional
13:41:48 gibi I'm fine with that extra conditional on the caller side
13:42:00 bauzas with kwargs
13:42:21 bauzas like kwargs = {}
13:42:29 gibi also I'm fine passing the sentinel and avoid the conditional too
13:42:35 bauzas api.unshelve(ctxt, inst, host, **kwargs)
13:42:52 gibi ahh, OK, you can do that too
13:42:56 bauzas and if az is set, then kwargs['new_az'] = this
13:43:09 bauzas that's the general pattern we have for sentinels
13:43:44 bauzas no need to have different calls then
13:43:52 gibi I'm OK with the kwargs way too
13:43:53 bauzas one single call but with optional args
13:44:17 bauzas gibi: well, as I said, this is the pattern we have in Nova for a while now :)
13:44:29 bauzas ie. default a param to a sentinel value
13:44:35 bauzas and call the method with kwargs
13:44:52 bauzas Uggla: ^
13:45:43 gibi I can argue about how well established this as a pattern based on I'm not being aware of it. But I guess it is irrelevant. :)
13:45:59 gibi as I agree with the actual code
13:47:12 bauzas https://github.com/openstack/nova/blob/master/nova/scheduler/rpcapi.py#L152
13:47:19 bauzas and https://github.com/openstack/nova/blob/master/nova/scheduler/manager.py#L145
13:47:40 bauzas gibi: the problem with importing a global var from another module is about any possible circular import
13:47:52 bauzas if we don't need to import, let's not do it
13:48:14 gibi sure
13:48:17 bauzas also, the caller has to have some knowledge about the method
13:48:29 gibi as I said I agree with the code :)
13:48:34 bauzas this is a bad behaviour
13:48:41 bauzas gibi: sure, I just explain why
13:48:56 bauzas mostly not for you
13:49:00 bauzas but also for Uggla :)
13:49:45 bauzas so, yeah, unnecessary module import and internal knowledge of the called method are the two things we want to avoid
13:50:11 bauzas and since optional params in python methods are something easy to do, let's use this pattern
13:53:14 Uggla hum I think I understand.
13:53:27 gibi I think we use None a lot more to signal information-not-provided than a sentinel value. We only use dedicated sentinel if None means something else than information-not-provided
13:53:47 gibi and in this case None means something else than information-not-provide4d
13:54:25 bauzas gibi: correct
13:54:44 bauzas in general, we set to None unless None is used for a specific flag
13:54:52 gibi yepp
13:54:56 bauzas and if so, we use the sentinel pattern
13:55:49 bauzas but again, we create a specific instance of an object that we use, and we try to not expose this instance elsewhere
13:56:13 bauzas by instance, I mean a stored value in memory
14:05:30 gibi purely hypotetically and purely from the code understandability perspective I'm not in favor of the optionalness of a parameter on an API. In python you have to look up the signature of the called function to know if a parameter is optional (i.e. has a default value in the signature) but as soon as you looked that up you see the default value (the sentinel) so that default value already leaked to the
14:05:36 gibi caller side
14:06:35 gibi I would make all the parameters non defaulted and document what value of what parameter means what :D
14:06:54 gibi that is a bit more explict than param=sentinel in a signature
14:07:16 gibi but this is way less important that make the unshelve patch land :D
14:07:24 gibi s/that/than/
14:07:42 bauzas that's why I explained why the pattern and also why I said in my last comment that docstrings help
14:08:12 bauzas I'd rather see a docstring saying (optional) my param
14:08:43 bauzas rather than asking to import a specific instance of a global class object
14:10:46 gibi the function itself is a global on the class an you import that to be able to call it
14:11:19 gibi None is an interpreter global :)
14:11:33 gibi (we are getting philosophycal )
14:12:33 gibi we are using enums from the fields module those are class level fields too
14:26:49 Uggla gibi, bauzas, last stuff, ok with FIELD_SENTINEL wording ?
14:29:48 gibi go with what bauzas asked for I won't block on it
14:30:29 bauzas Uggla: gibi: you can even name it privatly
14:30:36 bauzas like _unset_field
14:30:45 gibi sure
14:30:48 bauzas or _unsel_field_sentinel
14:31:01 bauzas I don't wanna nitpick on the naming :)
14:31:22 Uggla naming one of the hardest stuff in computing.
14:31:30 bauzas but yeah, actually, since this is an internal object, make it private
14:31:43 bauzas (by convention of course)
14:32:27 bauzas https://twitter.com/codinghorror/status/506010907021828096?lang=fr
14:32:31 bauzas :)
14:32:44 bauzas (I like this tweet :p )
14:33:38 bauzas https://www.karlton.org/2017/12/naming-things-hard/ for the wider context
17:11:44 opendevreview Amit Uniyal proposed openstack/nova master: Adds validation for hw machine type in host caps https://review.opendev.org/c/openstack/nova/+/847126
19:45:42 opendevreview Amit Uniyal proposed openstack/nova master: Adds validation for hw machine type in host caps https://review.opendev.org/c/openstack/nova/+/847126

Earlier   Later