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.

Sign up for GitHub

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

Jump to bottom

fix getting hash from redis db #465

Merged
merged 3 commits into from
Mar 19, 2021
Merged

fix getting hash from redis db #465

merged 3 commits into from
Mar 19, 2021

Conversation

dmytroxshevchuk
Copy link
Contributor

No description provided.

@dmytroxshevchuk dmytroxshevchuk mentioned this pull request Mar 10, 2021
Closed
qiluo-msft
qiluo-msft reviewed Mar 10, 2021
View reviewed changes
common/configdb.cpp
{
row = key.substr(pos + 1);
if (pos == string::npos) {
continue;
Copy link
Contributor

@qiluo-msft qiluo-msft Mar 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

continue [](start = 12, length = 8)

Thanks for the fix! Could you add a unit test? #Closed

Copy link

Choose a reason for hiding this comment

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

@qiluo-msft,
I added unit test for get_config() and get_table() functions. Please review.

@qiluo-msft
Copy link
Contributor

qiluo-msft commented Mar 10, 2021 •
edited
Loading 

    if (pos != string::npos)

This one is similar, however it is not likely to happen since the pattern has separator.
Could you also continue if separator not found? #Closed

Refers to: common/configdb.cpp:183 in a499586. [](commit_id = a499586, deletion_comment = False)

@qiluo-msft
Copy link
Contributor

qiluo-msft commented Mar 10, 2021 •
edited
Loading 

    if (pos != string::npos)

Could you also continue if not found? #Closed

Refers to: common/configdb.cpp:413 in a499586. [](commit_id = a499586, deletion_comment = False)

@mykolaxgerasymenko
Copy link 

    if (pos != string::npos)

This one is similar, however it is not likely to happen since the pattern has separator.
Could you also continue if separator not found?

Refers to: common/configdb.cpp:183 in a499586. [](commit_id = a499586, deletion_comment = False)

@qiluo-msft , continue was added if separator not found for both cases. Please review.

qiluo-msft
qiluo-msft previously approved these changes Mar 18, 2021
View reviewed changes
@dmytroxshevchuk
Copy link
Contributor Author

/AzurePipelines run

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 465 in repo Azure/sonic-swss-common

@mykolaxgerasymenko
Copy link

@qiluo-msft, I pushed with force without changes to restart automatic checks and tests.

@qiluo-msft qiluo-msft merged commit c2ccaa3 into sonic-net:master Mar 19, 2021
@allas-nvidia
Copy link

@qiluo-msft
Can you please merge it to 202012 as well. Thanks

@qiluo-msft
Copy link
Contributor

@daall Could you help cherry-pick to 202012 branch?

daall pushed a commit that referenced this pull request Mar 31, 2021
@dmytroxshevchuk @daall
fix getting hash from redis db (#465)
f59b572
@mint570 mint570 mentioned this pull request Apr 29, 2021
Merged
qiluo-msft pushed a commit that referenced this pull request May 11, 2021
@mint570
Fix config save error for non-hash key. (#485)
73d0e2d 
swss-common: Fix config save error for non-hash key

- Fixing #467
- Cleanup #465
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mykolaxgerasymenko mykolaxgerasymenko mykolaxgerasymenko left review comments

@qiluo-msft qiluo-msft qiluo-msft approved these changes

Assignees
No one assigned
Labels
Included in 202012 Branch Request for 202012 Branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dmytroxshevchuk @qiluo-msft @mykolaxgerasymenko @allas-nvidia @daall