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

[syncd] Fix bulk multi attrs for same key db update #761

Merged
merged 2 commits into from
Jan 5, 2021

Conversation

kcudnik
Copy link
Collaborator

@kcudnik kcudnik commented Jan 4, 2021

attributes will be set for the same key, then when we want to, push them to redis database, we need to combine all attributes, to a single vector of attributes

issue was related to bulk set operation, when multiple attributes were set for the same key, in this case bug was causing that only last attribute was actually written to database and previous ones were skipped

@kcudnik kcudnik added the Bug label Jan 4, 2021
yxieca
yxieca previously approved these changes Jan 4, 2021
syncd/Syncd.cpp Outdated
if (api == SAI_COMMON_API_BULK_SET)
{
// in case of bulk set operation, it can happen that multiple
// attributes will be set for the same key, then wehen we want to
Copy link
Contributor

Choose a reason for hiding this comment

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

wehen -> when

@lguohan
Copy link
Contributor

lguohan commented Jan 4, 2021

wonder if you add more description on the commit?

@kcudnik
Copy link
Collaborator Author

kcudnik commented Jan 4, 2021

done

@kcudnik
Copy link
Collaborator Author

kcudnik commented Jan 4, 2021

retest vs please

@kcudnik
Copy link
Collaborator Author

kcudnik commented Jan 4, 2021

@lguohan should all vs tests pass? in previous PR you mentioned that 4 tests are fine ?

@lguohan
Copy link
Contributor

lguohan commented Jan 4, 2021

yes, if you see 4 tests failing, then it means this pr is ok. the four failed test cases are known issue.

@lguohan
Copy link
Contributor

lguohan commented Jan 5, 2021

retest this please

@kcudnik
Copy link
Collaborator Author

kcudnik commented Jan 5, 2021

merging, since expected 4 tests failed

@kcudnik kcudnik merged commit e940136 into sonic-net:master Jan 5, 2021
@kcudnik kcudnik deleted the bulkfix branch January 5, 2021 13:18
pettershao-ragilenetworks pushed a commit to pettershao-ragilenetworks/sonic-sairedis that referenced this pull request Nov 18, 2022
issue was related to bulk set operation, when multiple attributes were set for the same key, in this case bug was causing that only last attribute was actually written to database and previous ones were skipped
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants