Support clearing the latest value in the LatestValueCache
#426
Conversation
This can be the overall latest value or the latest value for a specific key. This can be used to clean up old keys for which no further messages are expected. Signed-off-by: Sahas Subramanian <[email protected]>
shsms
added
the
cmd:skip-release-notes
|
No release notes, because this is an enhancement to an unreleased feature. |
There was a problem hiding this comment.
Pull Request Overview
Adds a clear method to reset either the overall latest value or the value for a specific key.
- Introduce
clear(key)to remove a key-specific latest value from the cache. - Default
clear()resets the overall latest value toNO_VALUE_RECEIVED.
Comments suppressed due to low confidence (1)
src/frequenz/channels/_latest_value_cache.py:197
- Add unit tests to verify that
clear()correctly resets the overall latest value and thatclear(key)only removes the entry for that key.
def clear(self, key: HashableT | Sentinel = NO_KEY) -> None:
Signed-off-by: Sahas Subramanian <[email protected]>
github-actions
bot
added
the
part:tests
Not entirely true, |
llucax
removed
the
cmd:skip-release-notes
|
I removed the skip notes label, I'm on the edge here, it is still a pretty small change so not putting it into the release notes is not the end of the world, but technically we should. If you want to add the label again and merge, I won't complain. |
Signed-off-by: Sahas Subramanian <[email protected]>
|
True, updated. |
| def clear(self, key: HashableT | Sentinel = NO_KEY) -> None: | ||
| """Clear the latest value or the latest value for a specific key. | ||
| If `key` is provided, it clears the latest value for that key. If no key is | ||
| provided, it clears the latest value received overall. | ||
| Args: | ||
| key: An optional key to clear the latest value for that key. If not | ||
| provided, it clears the latest value received overall. | ||
| """ | ||
| if not isinstance(key, Sentinel): | ||
| _ = self._latest_value_by_key.pop(key, None) | ||
| return | ||
|
|
||
| self._latest_value = NO_VALUE_RECEIVED | ||
|
|
There was a problem hiding this comment.
Why call clear() doesn't remove all keys and values in self._latest_value_by_key ?
Or shouldn't you remove _latest_value_by_key[self._key(self._latest_value)]], too?
There was a problem hiding this comment.
oh damn, because my usecase didn't need clearing all keys. I wonder if we should change the API slightly to keep the option open for a "clear everything" in the future.
any opinions @llucax ?
There was a problem hiding this comment.
Another option is to make the key parameter required, so that I can make progress and we can decide how to generalise later.
There was a problem hiding this comment.
Some options I can think of:
clear_all(),clear()but it is true thatclear()with no key looks ambiguousclear_all(),clear_key()maybe is more clear that without a key it clears the no-key?
I'm starting to find the use case for a "global" key a bit weird. I guess we need to support it for backwards compatibility, otherwise maybe we should always require a key and let the user use something like None if they don't want one? Not sure.
There was a problem hiding this comment.
We haven't released it. I can actually pull out all the new stuff into experimental.GroupingLatestValueCache that doesn't have a global mode.
|
This will go away once #428 is merged. |
3 participants
This can be the overall latest value or the latest value for a
specific key. This can be used to clean up old keys for which no
further messages are expected.