Change redisselect to use the same DBConnector object to subscribe to multiple patterns#1079
Change redisselect to use the same DBConnector object to subscribe to multiple patterns#1079gpunathilell wants to merge 1 commit intosonic-net:masterfrom
Conversation
Signed-off-by: Vivek Reddy <vkarri@nvidia.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@qiluo-msft This is the PR which was created based on the discussion with Vivek and Sudharsan (pubsub improvement) Please review when you have time. Thank you |
| self.pubsub = self.get_redis_client(self.db_name).pubsub() | ||
|
|
||
| def subscribe(self, table, handler, fire_init_data=False): | ||
| self._ensure_pubsub() |
There was a problem hiding this comment.
There is one pitfall in this solution. The redis connections are not thread safe, so each connection should be used inside each thread only. If multiple thread calling subscribe, they will share the same pubsub, which will not work.
you may consider thread local variables if you really want to achieve the optimization.
There was a problem hiding this comment.
Hi @qiluo-msft , Is the existing solution thread safe? And would there be instances where the implementation is done in this way (with multi-thread subscribe calls)?
There was a problem hiding this comment.
The redis connections are not thread safe
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| return string("value ") + to_string(index) + ":" + to_string(keyid); | ||
| } | ||
|
|
||
| static inline void clearDB() |
Check notice
Code scanning / CodeQL
Unused static function Note test
|
/azpw run |
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
4 participants
Update pubsub class to use the same dbconnector class and use it for ConfigDbConnector listen, so that it can subscribe to multiple patterns at the same time, thereby causing it to be used only once
Test results