Add selector feature #13
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #13 +/- ##
==========================================
+ Coverage 95.00% 95.72% +0.72%
==========================================
Files 10 11 +1
Lines 540 632 +92
==========================================
+ Hits 513 605 +92
Misses 27 27 ☔ View full report in Codecov by Sentry. |
src/pytroll_watchers/selector.py
Outdated
|
|
||
| usage: pytroll-selector [-h] [-l LOG_CONFIG] config | ||
|
|
||
| Selects unique messages from multiple sources. |
There was a problem hiding this comment.
Should there be a mention what defines "uniqueness"?
There was a problem hiding this comment.
Clarified in the help message and in the module docstring.
src/pytroll_watchers/selector.py
Outdated
| messages. | ||
|
|
||
| """ | ||
| with _running_redis_server(port=selector_config.get("port"), directory=selector_config.pop("directory", None)): |
There was a problem hiding this comment.
Can an existing Redis instance be used?
There was a problem hiding this comment.
Not that it is necessary to implement that option in this PR, just curious.
There was a problem hiding this comment.
In principle we could add the feature, yes. But I thought it would be best to keep the instances separate, not really being familiar with redis yet and how that could affect an existing instance.
src/pytroll_watchers/selector.py
Outdated
| key = msg.data["uid"] | ||
| try: | ||
| _ = sel[key] |
There was a problem hiding this comment.
I think the uniqueness check should be a separate function for clarity.
src/pytroll_watchers/selector.py
Outdated
| key = msg.data["uid"] | ||
| try: | ||
| _ = sel[key] | ||
| logger.info(f"Discarded {str(msg)}") |
There was a problem hiding this comment.
| logger.info(f"Discarded {str(msg)}") | |
| logger.debug(f"Discarded {str(msg)}") |
I think INFO is too high log level for discarding duplicates.
src/pytroll_watchers/selector.py
Outdated
|
|
||
|
|
||
| @contextmanager | ||
| def _running_redis_server(port=None, directory=None): |
There was a problem hiding this comment.
| def _running_redis_server(port=None, directory=None): | |
| def _start_redis_server(port=None, directory=None): |
To me running sounds like we are just connecting to an already running instance, not starting a new server process.
| At the moment, this module makes use of redis as a refined dictionary for keeping track of the received files. Hence a | ||
| redis server instance will be started along with some of the functions here, and with the cli. |
There was a problem hiding this comment.
Is the Redis database persisted? Where is it stored? If the database is not persisted, would a more lightweight solution (like dict) be an option?
There was a problem hiding this comment.
I haven't explored all of redis' possibilities, so I'm using the defaults now. The directory where the data is persisted can be defined through the directory parameter in the selector config (see docstrings).
I have looked for alternatives such as a dict with ttl for items but I haven't found anything that would work really.
This PR adds the selector feature and script for when multiple sources providing the same data are to be used.