Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added abstraction for bulk operations through redis pipeline #82

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

anilkpan
Copy link

For new commands to add/del range of vlans and range of vlan members, added new interfaces for bulk redis pipeline update

def set_bulk(self, payload):
"""Write bulk entries to config db.
"""
client = self.redis_clients[self.db_name]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to use the below API which is Multi-DB compliant.

self.get_redis_client(self.db_name). Modify other occurrences as well.

@rajendra-dendukuri
Copy link
Contributor

@qiluo-msft can you please review this change. These bulk API's improve performance when performing large number of table operations.

@@ -193,6 +193,42 @@ def set_entry(self, table, key, data):
k = k + '@'
client.hdel(_hash, self.serialize_key(k))

def set_bulk(self, payload):
Copy link
Contributor

@qiluo-msft qiluo-msft Jun 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

set_bulk [](start = 8, length = 8)

What are the use cases for the new functions? Application can achieve pipeline by calling get_redis_client().pipeline(). And that solution will be more flexible since you can mix functions into the same pipeline.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Qi, Please refer to the below comment from. It was suggested not to use the pipeline in the application.

sonic-net/sonic-utilities#891 (comment)

The use case is also mentioned in the same PR. It is to create large number of VLANs. This also helps when querying and displaying the contents of a filled-up mac address table.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @rajendra-dendukuri sharing the background! As we planed to move away from sonic-py-swsssdk and converge everything to sonic-swss-common, I believe new feature here will be short-lived. We have implemented some pipeline support in sonic-swss-common *Tables classes. Could you check if that fulfill your requirement?

Sorry for the inconvenience to extend code here.


In reply to: 445300357 [](ancestors = 445300357)

Copy link
Contributor

@qiluo-msft qiluo-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ask for use cases.

@rajendra-dendukuri
Copy link
Contributor

Ask for use cases.
Below is a usecase.
sonic-net/sonic-utilities#891

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants