Ensure dataset_lifetime bind is defined for DBS3Buffer/NewSubscription #11179
Conversation
|
Jenkins results:
|
| @@ -39,6 +39,8 @@ def _createPhEDExSubBinds(self, datasetID, subscriptionInfo, custodialFlag): | |||
|
|
|||
| # DeleteFromSource is not supported for move subscriptions | |||
| delete_blocks = None | |||
| # if None, force it to 0 as per the table schema | |||
| dataLifetime = subscriptionInfo['DatasetLifetime'] or 0 | |||
There was a problem hiding this comment.
I prefer this syntax: dataLifetime = subscriptionInfo.get('DatasetLifetime', 0) It is safer since it checks if key exists in a dictionary, while in your case it can fail.
get dict key safely
|
Jenkins results:
|
| @@ -39,6 +39,8 @@ def _createPhEDExSubBinds(self, datasetID, subscriptionInfo, custodialFlag): | |||
|
|
|||
| # DeleteFromSource is not supported for move subscriptions | |||
| delete_blocks = None | |||
| # if None, force it to 0 as per the table schema | |||
| dataLifetime = subscriptionInfo.get('DatasetLifetime', 0) or 0 | |||
There was a problem hiding this comment.
This line is quite ambiguous, I think. The .get() method is returning a proper default value, but we'd better have an explicit check for None value.
There was a problem hiding this comment.
@todor-ivanov you can see for your self:
# if you don't provide default value the type is None
>>> d={}
>>> d.get("foo")
>>> r=d.get("foo")
>>> type(r)
<class 'NoneType'>
# while if you provide default value the type is type of that value
>>> r=d.get("foo", 0)
>>> type(r)
<class 'int'>
Therefore, the line is correct, i.e. it is always return integer.
There was a problem hiding this comment.
Thanks @vkuznet
I do see what the line is doing. I did not mean it is wrong. I simply wanted to say: The reader may just miss the fact that there is also another possibility the value to be None at first place - provided as such value from the dictionary itself. But as written here also works perfectly fine. Lets not waste time on such minor things. Forget about my comment @amaltaro. The code is good.
|
Thanks for the review! |
Fixes #11178
Status
ready
Description
Make sure that bind
dataset_lifetimeis created when the SQL statement is executed. In case it's not defined, default to the same default value defined in the table schema creation (integer 0).Is it backward compatible (if not, which system it affects?)
YES
Related PRs
Fixes a bug added in #11115
External dependencies / deployment changes
None