DM-42676: Fix quantum backed butler trust mode with chained datastore #951
Conversation
| @@ -1116,6 +1116,11 @@ def transfer_from( | |||
| # child datastores. | |||
| source_datastores = (source_datastore,) | |||
|
|
|||
| refs = list(refs) | |||
There was a problem hiding this comment.
This seems wrong to me but we've declared refs to be Iterable and that doesn't support len.
| @@ -1116,6 +1116,11 @@ def transfer_from( | |||
| # child datastores. | |||
| source_datastores = (source_datastore,) | |||
|
|
|||
| refs = list(refs) | |||
| if len(refs) == 0: | |||
There was a problem hiding this comment.
if not refs:does pass mypy for an Iterator but not sure if that's safe.
There was a problem hiding this comment.
If refs is a list then not refs and len(refs) == 0 work identically.
There was a problem hiding this comment.
Yes. That's my point, but in the more general case for Iterator am I setting myself up for pain if I do that. I am wondering if we should make the type annotation be Collection instead and therefore guarantee that len works, or do something like:
if not isinstance(refs, Sized):
refs = list(refs)There was a problem hiding this comment.
If we need len then Collection annotation is probably better.
timj
force-pushed
the
tickets/DM-42676
branch
from
d95472e
to
87728a7
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #951 +/- ##
==========================================
- Coverage 88.41% 88.40% -0.01%
==========================================
Files 303 303
Lines 39137 39128 -9
Branches 8244 8245 +1
==========================================
- Hits 34602 34591 -11
+ Misses 3341 3330 -11
- Partials 1194 1207 +13 ☔ View full report in Codecov by Sentry. |
| @@ -1116,6 +1116,11 @@ def transfer_from( | |||
| # child datastores. | |||
| source_datastores = (source_datastore,) | |||
|
|
|||
| refs = list(refs) | |||
| if len(refs) == 0: | |||
There was a problem hiding this comment.
If refs is a list then not refs and len(refs) == 0 work identically.
| for this_datastore in getattr(datastore, "datastores", [datastore]): | ||
| if hasattr(this_datastore, "trustGetRequest"): | ||
| this_datastore.trustGetRequest = True |
There was a problem hiding this comment.
So, if by some chance non-chained datastore adds datastores attribute, we won't try to set its trustGetRequest? And it will also crash if datastore.datastores is not an Iterable?
There was a problem hiding this comment.
If datastores is not an iterable we are in trouble indeed. I was pondering adding datastores to Datastore and making it return [self] by default. Then it would be clear that it has a special purpose that only something like ChainedDatastore can override.
There was a problem hiding this comment.
Maybe just add "private" method _set_trust_mode() to the base class?
There was a problem hiding this comment.
I will do that. I wasn't sure I wanted to make it something that top-level given we can't think of anything outside FileDatastore caring.
Can not assume we only have one file datastore.
Method makes it easier for quantum backed butler and tests to change the flag.
timj
force-pushed
the
tickets/DM-42676
branch
from
6142e59
to
7f485cf
Compare
Checklist
doc/changes