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

[orch] change Consumer class to support multiple values for the same key #1184

Merged
merged 4 commits into from
Jan 29, 2020

Conversation

zhenggen-xu
Copy link
Collaborator

Description

The Consumer class is used by the orch objects to deal with the redis consumer tables' popped items. It has m_toSync map to save the tasks. During the operations (more items than the orch objects can handle), the tasks in the map is merged for optimization. However, since it is a map, we would only have one value for each key. This potentially eliminate the necessary actions from Redis, e,g, we have a DEL action and SET action coming out from Redis for some key, we would overwrite the DEL action today in the code and thus not able to delete some objects as we intended.

The PR changed the m_toSync from map to multi-map to get multiple values for one key. In this design, we keep maximun two values per key, DEL or SET or DEL+SET. We need strictly keep the order of DEL and SET. It is possible to use map of vectors to fulfill this, we chose multi-map because:
1, It will have less/no changes to different orch classes to iterate the m_toSync
2, The order can be guaranteed. The order of the key-value pairs whose keys compare equivalent is the order of insertion and does not change. (since C++11). See
https://en.cppreference.com/w/cpp/container/multimap

The PR also refactors the consumer class so vlanmgr.cpp and routeorch.cpp will leverage the Consumer functions instead of operating on the members. It also refactors the UT code (aclorch_ut.cpp) so it removes the redundant code and uses the same code.

Google UT tests were added for Consumer Class especially for different cases for addToSync() function.

What I did
Change the m_toSync in Consumer class to multimap so it could support both DEL and SET
Reload Consumer addToSync() and refactor vlanmgr/route-orch and ut code to use it
Add google ut for consumer class

Why I did it
See description.

How I verified it
Unit tests:
Running main() from gtest_main.cc

[==========] Running 19 tests from 5 test cases.
[----------] Global test environment set-up.
[----------] 1 test from AclTest
[ RUN      ] AclTest.Create_L3_Acl_Table
[       OK ] AclTest.Create_L3_Acl_Table (1 ms)
[----------] 1 test from AclTest (1 ms total)

[----------] 3 tests from AclOrchTest
[ RUN      ] AclOrchTest.ACL_Creation_and_Destorying
[       OK ] AclOrchTest.ACL_Creation_and_Destorying (1000 ms)
[ RUN      ] AclOrchTest.L3Acl_Matches_Actions
[       OK ] AclOrchTest.L3Acl_Matches_Actions (1001 ms)
[ RUN      ] AclOrchTest.L3V6Acl_Matches_Actions
[       OK ] AclOrchTest.L3V6Acl_Matches_Actions (1000 ms)
[----------] 3 tests from AclOrchTest (3003 ms total)

[----------] 2 tests from PortsOrchTest
[ RUN      ] PortsOrchTest.PortReadinessColdBoot
[       OK ] PortsOrchTest.PortReadinessColdBoot (21 ms)
[ RUN      ] PortsOrchTest.PortReadinessWarmBoot
[       OK ] PortsOrchTest.PortReadinessWarmBoot (13 ms)
[----------] 2 tests from PortsOrchTest (34 ms total)

[----------] 4 tests from SaiSpy
[ RUN      ] SaiSpy.CURD
[       OK ] SaiSpy.CURD (0 ms)
[ RUN      ] SaiSpy.Same_Function_Signature_In_Same_API_Table
[       OK ] SaiSpy.Same_Function_Signature_In_Same_API_Table (0 ms)
[ RUN      ] SaiSpy.Same_Function_Signature_In_Different_API_Table
[       OK ] SaiSpy.Same_Function_Signature_In_Different_API_Table (0 ms)
[ RUN      ] SaiSpy.create_switch_and_acl_table
[       OK ] SaiSpy.create_switch_and_acl_table (0 ms)
[----------] 4 tests from SaiSpy (0 ms total)

[----------] 9 tests from ConsumerTest
[ RUN      ] ConsumerTest.ConsumerAddToSync_Set
[       OK ] ConsumerTest.ConsumerAddToSync_Set (1 ms)
[ RUN      ] ConsumerTest.ConsumerAddToSync_Del
[       OK ] ConsumerTest.ConsumerAddToSync_Del (0 ms)
[ RUN      ] ConsumerTest.ConsumerAddToSync_Set_Del
[       OK ] ConsumerTest.ConsumerAddToSync_Set_Del (0 ms)
[ RUN      ] ConsumerTest.ConsumerAddToSync_Del_Set
[       OK ] ConsumerTest.ConsumerAddToSync_Del_Set (130 ms)
[ RUN      ] ConsumerTest.ConsumerAddToSync_Set_Del_Set_Multi
[       OK ] ConsumerTest.ConsumerAddToSync_Set_Del_Set_Multi (204 ms)
[ RUN      ] ConsumerTest.ConsumerAddToSync_Set_Del_Set_Multi_In_Q
[       OK ] ConsumerTest.ConsumerAddToSync_Set_Del_Set_Multi_In_Q (4 ms)
[ RUN      ] ConsumerTest.ConsumerAddToSync_Del_Set_Setnew
[       OK ] ConsumerTest.ConsumerAddToSync_Del_Set_Setnew (0 ms)
[ RUN      ] ConsumerTest.ConsumerAddToSync_Del_Set_Setnew1
[       OK ] ConsumerTest.ConsumerAddToSync_Del_Set_Setnew1 (0 ms)
[ RUN      ] ConsumerTest.ConsumerAddToSync_Ind_Set_Del
[       OK ] ConsumerTest.ConsumerAddToSync_Ind_Set_Del (0 ms)
[----------] 9 tests from ConsumerTest (340 ms total)

[----------] Global test environment tear-down
[==========] 19 tests from 5 test cases ran. (4344 ms total)
[  PASSED  ] 19 tests.

Details if related

reload Consumer addToSync() and refactor ut code to use it

Add google ut for consumer class

Signed-off-by: Zhenggen Xu <[email protected]>
Signed-off-by: Zhenggen Xu <[email protected]>
@zhenggen-xu zhenggen-xu marked this pull request as ready for review January 28, 2020 23:34
cfgmgr/vlanmgr.cpp Outdated Show resolved Hide resolved
orchagent/orch.cpp Outdated Show resolved Hide resolved
orchagent/orch.cpp Outdated Show resolved Hide resolved
orchagent/orch.cpp Outdated Show resolved Hide resolved
orchagent/routeorch.cpp Outdated Show resolved Hide resolved
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.

Could you also change the title as a bug fix statement?

@qiluo-msft qiluo-msft self-assigned this Jan 29, 2020
@lguohan
Copy link
Contributor

lguohan commented Jan 29, 2020

i am ok with this pr.

lguohan
lguohan previously approved these changes Jan 29, 2020
Signed-off-by: Zhenggen Xu <[email protected]>
@zhenggen-xu zhenggen-xu changed the title [orch] Change the m_toSync in Consumer class to support both DEL and SET [orch] change Consumer class to support multiple values for the same key Jan 29, 2020
@zhenggen-xu
Copy link
Collaborator Author

@qiluo-msft Feedback was addressed.

@lguohan lguohan merged commit 881d742 into sonic-net:master Jan 29, 2020
lguohan pushed a commit that referenced this pull request Jan 30, 2020
…key (#1184)

Description

The Consumer class is used by the orch objects to deal with the redis consumer tables' popped items. It has m_toSync map to save the tasks. During the operations (more items than the orch objects can handle), the tasks in the map is merged for optimization. However, since it is a map, we would only have one value for each key. This potentially eliminate the necessary actions from Redis, e,g, we have a DEL action and SET action coming out from Redis for some key, we would overwrite the DEL action today in the code and thus not able to delete some objects as we intended.

The PR changed the m_toSync from map to multi-map to get multiple values for one key. In this design, we keep maximun two values per key, DEL or SET or DEL+SET. We need strictly keep the order of DEL and SET. It is possible to use map of vectors to fulfill this, we chose multi-map because:
1, It will have less/no changes to different orch classes to iterate the m_toSync
2, The order can be guaranteed. The order of the key-value pairs whose keys compare equivalent is the order of insertion and does not change. (since C++11). See
https://en.cppreference.com/w/cpp/container/multimap

The PR also refactors the consumer class so vlanmgr.cpp and routeorch.cpp will leverage the Consumer functions instead of operating on the members. It also refactors the UT code (aclorch_ut.cpp) so it removes the redundant code and uses the same code.

Google UT tests were added for Consumer Class especially for different cases for addToSync() function.

What I did
Change the m_toSync in Consumer class to multimap so it could support both DEL and SET
Reload Consumer addToSync() and refactor vlanmgr/route-orch and ut code to use it
Add google ut for consumer class

Why I did it
See description.

How I verified it
Unit tests:
Running main() from gtest_main.cc

```
[==========] Running 19 tests from 5 test cases.
[----------] Global test environment set-up.
[----------] 1 test from AclTest
[ RUN      ] AclTest.Create_L3_Acl_Table
[       OK ] AclTest.Create_L3_Acl_Table (1 ms)
[----------] 1 test from AclTest (1 ms total)

[----------] 3 tests from AclOrchTest
[ RUN      ] AclOrchTest.ACL_Creation_and_Destorying
[       OK ] AclOrchTest.ACL_Creation_and_Destorying (1000 ms)
[ RUN      ] AclOrchTest.L3Acl_Matches_Actions
[       OK ] AclOrchTest.L3Acl_Matches_Actions (1001 ms)
[ RUN      ] AclOrchTest.L3V6Acl_Matches_Actions
[       OK ] AclOrchTest.L3V6Acl_Matches_Actions (1000 ms)
[----------] 3 tests from AclOrchTest (3003 ms total)

[----------] 2 tests from PortsOrchTest
[ RUN      ] PortsOrchTest.PortReadinessColdBoot
[       OK ] PortsOrchTest.PortReadinessColdBoot (21 ms)
[ RUN      ] PortsOrchTest.PortReadinessWarmBoot
[       OK ] PortsOrchTest.PortReadinessWarmBoot (13 ms)
[----------] 2 tests from PortsOrchTest (34 ms total)

[----------] 4 tests from SaiSpy
[ RUN      ] SaiSpy.CURD
[       OK ] SaiSpy.CURD (0 ms)
[ RUN      ] SaiSpy.Same_Function_Signature_In_Same_API_Table
[       OK ] SaiSpy.Same_Function_Signature_In_Same_API_Table (0 ms)
[ RUN      ] SaiSpy.Same_Function_Signature_In_Different_API_Table
[       OK ] SaiSpy.Same_Function_Signature_In_Different_API_Table (0 ms)
[ RUN      ] SaiSpy.create_switch_and_acl_table
[       OK ] SaiSpy.create_switch_and_acl_table (0 ms)
[----------] 4 tests from SaiSpy (0 ms total)

[----------] 9 tests from ConsumerTest
[ RUN      ] ConsumerTest.ConsumerAddToSync_Set
[       OK ] ConsumerTest.ConsumerAddToSync_Set (1 ms)
[ RUN      ] ConsumerTest.ConsumerAddToSync_Del
[       OK ] ConsumerTest.ConsumerAddToSync_Del (0 ms)
[ RUN      ] ConsumerTest.ConsumerAddToSync_Set_Del
[       OK ] ConsumerTest.ConsumerAddToSync_Set_Del (0 ms)
[ RUN      ] ConsumerTest.ConsumerAddToSync_Del_Set
[       OK ] ConsumerTest.ConsumerAddToSync_Del_Set (130 ms)
[ RUN      ] ConsumerTest.ConsumerAddToSync_Set_Del_Set_Multi
[       OK ] ConsumerTest.ConsumerAddToSync_Set_Del_Set_Multi (204 ms)
[ RUN      ] ConsumerTest.ConsumerAddToSync_Set_Del_Set_Multi_In_Q
[       OK ] ConsumerTest.ConsumerAddToSync_Set_Del_Set_Multi_In_Q (4 ms)
[ RUN      ] ConsumerTest.ConsumerAddToSync_Del_Set_Setnew
[       OK ] ConsumerTest.ConsumerAddToSync_Del_Set_Setnew (0 ms)
[ RUN      ] ConsumerTest.ConsumerAddToSync_Del_Set_Setnew1
[       OK ] ConsumerTest.ConsumerAddToSync_Del_Set_Setnew1 (0 ms)
[ RUN      ] ConsumerTest.ConsumerAddToSync_Ind_Set_Del
[       OK ] ConsumerTest.ConsumerAddToSync_Ind_Set_Del (0 ms)
[----------] 9 tests from ConsumerTest (340 ms total)

[----------] Global test environment tear-down
[==========] 19 tests from 5 test cases ran. (4344 ms total)
[  PASSED  ] 19 tests.
```

Signed-off-by: Zhenggen Xu <[email protected]>
oleksandrivantsiv pushed a commit to oleksandrivantsiv/sonic-swss that referenced this pull request Mar 1, 2023
* c0bdac2 2021-01-24 | [saithrift]: add correct libs to build saithrift on libsaivs (sonic-net#1184) (HEAD, origin/v1.7) [lguohan]

Signed-off-by: Guohan Lu <[email protected]>
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.

3 participants