bluesky / event-model

data model for event-based data collection and analysis
https://blueskyproject.io/event-model
BSD 3-Clause "New" or "Revised" License
15 stars 31 forks source link

Apply retry logic to handler instantiation. #135

Closed danielballan closed 4 years ago

danielballan commented 4 years ago

We currently use a retry loop (with configurable time-spacing and number of retries) when exchanging a Datum for a data payload. We should do the same when exchanging a Resource for handler instance because that step can and often does involve file I/O as well.

This factors out the retry loop into an internal utility function.

The resource health validator at FXI uses filler.get_handler() to check that the HDF5 files are at least open-able. It is happening a little too early and getting a false negative. Retries should fix it.

Tests were added targeting the newly-factored-out retry loop.

mrakitin commented 4 years ago

xref https://github.com/bluesky/resource-health-check

danielballan commented 4 years ago

Oh, you are right that there was bug. I think that your proposed solution had a different bug, though. If retry_intervals=None is given by the user, that code would have first set self._retry_intervals to [] (good) and then immediately set it again to None (oops). Maybe this safer approach is to perform the None-check on the local variable:

if retry_intervals is None:
    retry_intervals = []

and then rely on the property setter to convert any non-list iterables to lists

self.retry_intervals = retry_intervals  # the @retry_intervals.setter does `list(retry_intervals)` 

In the None case you end up doing a tiny bit of extra work there---list([])--- but the action is clear. I added a test normalizing a range of inputs.

mrakitin commented 4 years ago

The problem is that we don't initialize the self._retry_intervals, which is used in the properties methods. That's why I suggested the change: https://github.com/bluesky/event-model/pull/135/files#r369298908. Do you think it makes sense, @danielballan?

danielballan commented 4 years ago

Because we invoke the setter in __init__, which sets self._retry_intervals, we guarantee that no one can attempt to use the getter before self._retry_intervals is initialized.

mrakitin commented 4 years ago

I think I got it now.