Make sairedis/syncd synchronous#476
Conversation
| */ | ||
|
|
||
| recordLine("C|" + str_object_type + joined); | ||
| recordLine("R|" + str_object_type + joined); |
There was a problem hiding this comment.
R [](start = 20, length = 1)
Is there a wiki page to summarize all the special char used in recording? If not, please create one.
Alternatively, how about centralized the definition in one header file and use constants or macros in code?
There was a problem hiding this comment.
Could we set such a rule that, small letter means request, capital letter is for response?
Ex:
g/G(get request/Get response),
c/C (create request/create reponse).
r/R(remove request/Remove response).
s/S(set request/Set reponse)
b/B(bulk create/Bulk create response)
e/E(bulk remove request/Bulk remove response)
m/M(bulk set request/Buld set reponse)
There was a problem hiding this comment.
actually m/M should be removed since it's not SAI api call, its's request for counters subscription now, and recording should log only sai call's
also single 'b' is not good enough since you can have bulk remove, bulk create and so on
There was a problem hiding this comment.
Thanks @jipanyang ! I still suggest to maintain it as any of the below options:
- code comment or Github wiki page and referred by code comment.
- centralized the definition in one header file and use constants/macros/enums in code?
Nowadays it is hard to find, and easy to conflict.
| #include "swss/tokenize.h" | ||
| #pragma GCC diagnostic push | ||
| #pragma GCC diagnostic ignored "-Wshadow" | ||
| #include "swss/json.hpp" |
There was a problem hiding this comment.
Add pragma into header file directly so others could also benefit? #Closed
There was a problem hiding this comment.
this is in swss-common repo, also new update to json would need to keep this but sure i can add
There was a problem hiding this comment.
any follow up? #Closed
There was a problem hiding this comment.
jipanyang
left a comment
jipanyang
left a comment
There was a problem hiding this comment.
Do you have the performance data comparison before and after the change? I'm mostly concerned with neighbor and route related operations.
jipanyang
left a comment
jipanyang
left a comment
There was a problem hiding this comment.
Another question is have you considered using NotificationProducer instead of producer? internal_api_wait_for_response() will wait for the response with timeout limit, function wise there should not be much difference.
While working on #315 , it was noticed that NotificationProducer/NotificationConsumer channel has lower overhead than Producer/Consumer channel ( about 50%). Not sure if there is any change since then.
|
@jipanyang yes i have performance, send in email |
| #include <linux/if_ether.h> | ||
|
|
||
| #pragma GCC diagnostic push | ||
| #pragma GCC diagnostic ignored "-Wshadow" |
There was a problem hiding this comment.
it's separated from this PR;
meanwhile, i'm wondering if we want to upgrade the version of this json.hpp file to a newer one.
There was a problem hiding this comment.
if i remember there were some compiling issues with newer one thats why we stick with this one but you can take a look, im adding this since gcc 9.1 is complaining
|
can you make this optional so that we do not need to turn it on by default for now? |
|
Do you mean compile time optional or dynamic optional ? |
* Make sairedis/syncd synchronous * Add wait for response for bulk api * Update aspell * Address comments * Add support for bulk route create in saiplayer * Fix spelling * Make synchronous mode optional and disabled by default
5 participants
No description provided.