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

[sairedis] Add get response timeout knob #774

Merged
merged 4 commits into from
Feb 8, 2021
Merged

Conversation

kcudnik
Copy link
Collaborator

@kcudnik kcudnik commented Jan 22, 2021

Knob needed by mlnx when doing firmware update on some platforms, which can exceed default 1 min timeout.
Need to be set by orchagent.

@lguohan
Copy link
Contributor

lguohan commented Jan 22, 2021

can this be set on-the-fly?

@kcudnik
Copy link
Collaborator Author

kcudnik commented Jan 22, 2021

yes it can

@kcudnik
Copy link
Collaborator Author

kcudnik commented Jan 24, 2021

retest vs please

* @flgs CREATE_AND_SET
* @default 60000
*/
SAI_REDIS_SWITCH_ATTR_GET_RESPONSE_TIMEOUT_MS,
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe it should be SET_RESPONSE_TIMEOUT_MS

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

mainly this timeout was designed for GET api in any quad api, but with adding synchronous mode it's used for all of them

@@ -4,9 +4,12 @@

using namespace sairedis;

#define REDIS_ASIC_STATE_COMMAND_GETRESPONSE_TIMEOUT_MS (60*1000)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would like to use this define in sonic-swss/orchagent/saihelper.cpp
Can you please move this define to .h file that can be included from both Channel.cpp and saihelper.cpp?
If there is such...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

why would you need this define on saihelper?

Copy link
Contributor

Choose a reason for hiding this comment

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

I am using the new API to change the timeout and later on I want to change it back to default, this is where I want to use your define.
In the PR bellow I used new define SAI_REDIS_GET_RESPONSE_TIMEOUT_MS_DEFAULT but I would like to replace it to REDIS_ASIC_STATE_COMMAND_GETRESPONSE_TIMEOUT_MS if it is possible.
[(https://github.com/liorghub/sonic-swss/pull/3)]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i was actually thinking, that this define maybe should not be exposed as define here, but you should do SAI api GET api, to obtain this attribute default value

Copy link
Contributor

Choose a reason for hiding this comment

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

Generally, if we have set API we can have get API, basic stuff.
However, there is nothing wrong with exposing the default value in header file and for my needs it will do the work just fine. I usually vote for the option with less lines of code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok i leave it as is

@liat-grozovik
Copy link
Collaborator

@liorghub and @nazariig can you please approve so we can move forward and merge?

Copy link
Contributor

@liorghub liorghub left a comment

Choose a reason for hiding this comment

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

LGTM

@kcudnik kcudnik merged commit be8059f into sonic-net:master Feb 8, 2021
@kcudnik kcudnik deleted the timeout branch February 8, 2021 09:58
kktheballer pushed a commit to kktheballer/sonic-sairedis that referenced this pull request Mar 10, 2021
Knob needed by mlnx when doing firmware update on some platforms, which can exceed default 1 min timeout.
pettershao-ragilenetworks pushed a commit to pettershao-ragilenetworks/sonic-sairedis that referenced this pull request Nov 18, 2022
Knob needed by mlnx when doing firmware update on some platforms, which can exceed default 1 min timeout.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants