Posted	Nick	Remark
#openstack-nova - 2022-06-22
12:04:31	mnasiadka	gibi: oh ok, thanks
12:21:45	bauzas	gibi: Uggla: sean-k-mooney : can we punt our session by 30 mins ? I have something at the moment
12:22:04	sean-k-mooney	i wont be able to do it sorry
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 )

Earlier   Later