Implement a Redis backend storage #106
Conversation
chadell
changed the title
chadell
changed the title
|
@glennmatthews I found an issue with this PR, specifically on the handling of For instance, from example 01: ...
self.add(device)
site.add_child(device)
...In this example, ...
self.add(device)
site.add_child(device)
self.update(site)
...I have tested it and if we do the Looking forward to your thoughts |
pyproject.toml
Outdated
| @@ -23,6 +23,8 @@ packaging = "^21.3" | |||
| colorama = {version = "^0.4.3", optional = true} | |||
| # For Pydantic | |||
| dataclasses = {version = "^0.7", python = "~3.6"} | |||
| redis = "*" | |||
There was a problem hiding this comment.
- Is there a known minimum version of
redisthat's required? - Can we make this into an optional/extras dependency, i.e. so that
pip install diffsyncdoesn't pull in redis (and only supports local storage as a result), butpip install diffsync[redis]does?
Co-authored-by: Glenn Matthews <[email protected]>
| """Init method for RedisStore.""" | ||
| super().__init__(*args, **kwargs) | ||
|
|
||
| if url and host and port: |
There was a problem hiding this comment.
Perhaps?
| if url and host and port: | |
| if url and (host or port): |
There was a problem hiding this comment.
if we do like this, then I have to remove the default port from the kwargs, that I thought it would be convenient. But, it could be also confusing... not sure
There was a problem hiding this comment.
OK, perhaps just if url and host then?
diffsync/store/__init__.py
Outdated
| @@ -72,6 +72,10 @@ def get_by_uids( | |||
| """ | |||
| raise NotImplementedError | |||
|
|
|||
| def _remove_item(self, modelname: str, uid: str): | |||
There was a problem hiding this comment.
I don't think I like having a private but abstract method. If this is something subclasses are responsible for implementing, it shouldn't be private, IMHO.
There was a problem hiding this comment.
yes, I remember a previous conversation about this public/private question :)
diffsync/store/__init__.py
Outdated
| self.remove(obj=child_obj, remove_children=remove_children) | ||
| except ObjectNotFound: | ||
| # Since this is "cleanup" code, log an error and continue, instead of letting the exception raise | ||
| self._log.error(f"Unable to remove child {child_id} of {modelname} {uid} - not found!") |
There was a problem hiding this comment.
It occurs to me we should probably be taking advantage of structlog better on this error message. Perhaps something like:
| self._log.error(f"Unable to remove child {child_id} of {modelname} {uid} - not found!") | |
| self._log.error(f"Unable to remove child element as it was not found!", child_type=child_type, child_id=child_id, parent_type=modelname, parent_id=uid) |
diffsync/store/redis.py
Outdated
| from redis import Redis | ||
| from redis.exceptions import ConnectionError as RedisConnectionError |
There was a problem hiding this comment.
Don't we need some logic here to handle the case where redis isn't installed?
This PRs addresses issue #57 partially, abstracting the store for DiffSyncModel objects, and extending to using Redis backend besides the local memory.
This feature it's specially important when working on a distributed data pipeline when different workers would load the data from adapters and finally the data must be shared to create the diff. It leverages on #58 proposal.
The implementation approach has moved the previous
_datainto theLocalStoreimplementation, creating abstract methods to interact with the storage, so we were able to create another implementation using Redis.TODO