Replace borg connection manager with thread safe db pool #593
Conversation
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ |
| self.port = port | ||
| self.db_name = db_name | ||
|
|
||
| def __enter__(self): |
There was a problem hiding this comment.
i don't think you need to change this method, the idea should be to replace the connection manager with sqlalchemy "create engine", but without changing interfaces
src/translators/crate.py
Outdated
| def __init__(self, host, port=4200, db_name="ngsi-tsdb"): | ||
| super(CrateTranslator, self).__init__(host, port, db_name) | ||
| def __init__(self, connection, query , host, port=4200, db_name="ngsi-tsdb"): | ||
| super(CrateTranslator, self).__init__(host, connection, query , port, db_name) |
src/translators/crate.py
Outdated
|
|
||
| def setup(self): | ||
| def setup(self, connection, query): | ||
| url = "{}:{}".format(self.host, self.port) |
There was a problem hiding this comment.
so, in here, you would use create_engine to get your connection using the proper parameters
| exc_info=True) | ||
|
|
||
|
|
||
| class QueryCacheManager(Borg): |
There was a problem hiding this comment.
i think you can keep the borg here
src/translators/sql_translator.py
Outdated
| def __init__(self): | ||
| super(QueryCacheManager, self).__init__() | ||
| def __init__(self, connection, query): | ||
| super(QueryCacheManager, self ).__init__( connection, query) |
There was a problem hiding this comment.
and leave this unchanged
There was a problem hiding this comment.
updated as suggested
src/utils/connection_manager.py
Outdated
| @@ -1,25 +1,67 @@ | |||
| class Borg: | |||
| _shared_state = {} | |||
| from crate import client | |||
There was a problem hiding this comment.
you should leave this class unchanged, because as said the idea is to replace directly in setup, secondly, you would need to proceed, so to say, in parallel and support both crate and timescale, easy if you keep this unchanged, if you touch this, you would need to adjust also timescale code, since it uses this code
src/translators/crate.py
Outdated
| def setup(self): | ||
| def setup(self, connection, query): | ||
| url = "{}:{}".format(self.host, self.port) | ||
| self.ccm = ConnectionManager() |
There was a problem hiding this comment.
here could be an example (but don't take granted it works ;) ):
url = "crate://{}:{}".format(self.host, self.port)
self.engine = sa.create_engine(url, connect_args={"pool_size": 10})
self.connection = self.engine.connect()
of course it could be possible to create the two connection pools (the one for crate and the one for timescale), at the app start, and pass them, this would be probably the best option from a tech point, but you would need probably to change quite some code...
There was a problem hiding this comment.
updated as suggested
There was a problem hiding this comment.
@chicco785 @c0c0n3 , I have PR as suggested but some test are failing which are not shown up as in my local env.
2 participants
Proposed changes
Does changes as per references
Fix for Issue
Types of changes
What types of changes does your code introduce to the project: Put
an
xin the boxes that applyfunctionality to not work as expected)
Checklist
Put an
xin the boxes that apply. You can also fill these out aftercreating the PR. If you're unsure about any of them, don't hesitate to
ask. We're here to help! This is simply a reminder of what we are going
to look for before merging your code.
feature works
downstream modules
Further comments
Replaced "borg" connection used in file connection_manager.py with
CrateDB's SQLAlchemy dialectAlso, took references from discussion
used
create_engineconnection.