Allow wait condition to access timestamps #39
Conversation
The current wait_key() condition only checks the value of a telstate item. Extend it to optionally check the associated timestamp of the item (if the key is mutable). This allows scripts to wait on telstate until a fresh value appears for an item. In the process, extend the redis pubsub update message format to match the raw sensor format used in the main key-value store. In other words, attributes have pickled values and sensors additionally prepend a packed timestamp to each pickled value. The timestamp parameter of the wait_key condition is optional so that this extension is backwards compatible. This exposed an ASCII encoding bug in fakeredis on Python 2, for which we include a monkey patch as workaround. Once fakeredis issue #146 is resolved the monkey patch can go away. This addresses JIRA ticket SR-965.
katsdptelstate/telescope_state.py
Outdated
| value, ts = self.get_range(key)[0] if str_val is None else self._strip(str_val) | ||
| try: | ||
| return condition(value, ts) | ||
| except TypeError: |
There was a problem hiding this comment.
I use TypeError to differentiate the two possible signatures of condition. Another option is inspect.getargspec (Python 2) and inspect.signature (Python 3) which can tell whether condition expects more than one parameter. Opinons welcome :-)
There was a problem hiding this comment.
I think there are very few places that wait_key is used, and several of them can probably be eliminated (waits on sdp_cam2telstate_ready are no longer needed). What about the following migration path:
- Remove any existing uses that aren't actually needed.
- For the remaining uses (if any), give the callback an optional (and unused) timestamp argument so that it can be called with or without a timestamp.
- Change telstate so that it always passes a timestamp.
That would avoid having to put this sort of magic into telstate.
There was a problem hiding this comment.
Hallelujah.
Quoting @bmerry from the now defunct #32:
This is not a backwards-compatible change. What is your plan to ensure that
(a) all callers of wait with a custom condition are switched when they land the new telstate; and
(b) all containers in a graph are running the same variant of the pubsub protocol?
This was my original migration path until you made me worry. I am glad your concerns have dissipated after I had addressed it :-)
|
I think at the time we had more users of wait_key, because we needed to wait for sdp_cam2telstate_status='ready' in a number of places. There are probably still some of those around, but they can be deleted. |
| """ | ||
| if str_val is None and not self._r.exists(key): | ||
| return False | ||
| if condition is None: |
There was a problem hiding this comment.
The test coverage around here needs to be improved: see the coverage report
Remove the hacky support for two types of condition callable and standardise on the most general one. It is now backwards compatible as no-one seems to use `wait_key` anymore... The observation scripts will use the new version soon, though.
The current
wait_key()condition only checks the value of a telstate item. Extend it to optionally check the associated timestamp of the item (if the key is mutable). This allows scripts to wait on telstate until a fresh value appears for an item.In the process, extend the redis pubsub update message format to match the raw sensor format used in the main key-value store. In other words, attributes have pickled values and sensors additionally prepend a packed timestamp to each pickled value.
The timestamp parameter of the
wait_keycondition is optional so that this extension is backwards compatible.This exposed an ASCII encoding bug in fakeredis on Python 2, for which we include a monkey patch as workaround. Once fakeredis issue #146 is resolved the monkey patch can go away.
This is a rehash of #32, which outgrew itself and then stalled.
This addresses JIRA ticket SR-965.