Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[debug dump util] Module implementation Logic and Port Module #1667
[debug dump util] Module implementation Logic and Port Module #1667
Changes from 21 commits
cb06a3b
d236a7d
f5ffd27
cef2433
bfe107e
e6607da
ee22710
6d094f4
ffe6158
b750eba
b92290e
6d76514
244ff65
483cc52
826ded0
0d42722
63f9aa4
06b8297
b8d2421
94eedcc
0db4c12
8dac306
dc43339
1eb3be8
663f46b
0082da2
2ea3725
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we need to introduce a new class for mocking Sonicv2connector. Can we add new functionality to
dbconnector.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing Use case for these modules is to read from a separate json files as opposed to other tests which read from the default json files inside mock_tables directory.
I did see there is a dedicated_db list used inside one of the dbconnector methods
The only test which uses this is db_migrator_test.py. However in that test, it was initializing the SonicV2Connector class directly inside the test, whereas in dump_module_tests it is being initialized in the methods defined in the actual classes.
And probably because of the complex redirection used in the dbconnector.py to mock the SonicV2Connector, i was not able to get the dump_module_tests tests running on custom json files. I've also worked with stephan who introduced that change but couldn't get it running.
Hence, decided to add a new mock class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found a couple of examples of using the
dedicated_dbs
recently added recentlyhttps://github.com/Azure/sonic-utilities/blob/master/tests/config_int_ip_test.py
https://github.com/Azure/sonic-utilities/blob/5002745beb89a99a1cdf420f34a495c2c0cb31bd/tests/mpls_test.py#L60-L67
Can you check if we can follow a similar approach here?
I feel we should try and avoid duplicate mock classes if possible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the info. I was able to remove the extra mock and use the default mock. I couldn't get it to directly read the custom json files, but i've handled it nevertheless.
Regarding the connection pooling optimization, i've recently observed when operating at scale, (i.e. route, had around 10k routes to parse for the dump utility) i've observed a significant amount of time spent in connect calls. With this optimization, the number of connect calls can be effectively reduced from (number_of_routes*number_of_calls_per_route) to 4 (number of db's the module is looking into) and thus offering a significant improvement in performance.
This design change infact helped me to use the default mock as well.
@SuvarnaMeenakshi and @arlakshm , please provide your comments on the changes.