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

Add support of 'with' statement to ConfigDBConnector #838

Merged
merged 11 commits into from
Jan 19, 2024

Conversation

liuh-80
Copy link
Contributor

@liuh-80 liuh-80 commented Nov 30, 2023

Why I did it

Add support of 'with' statement to ConfigDBConnector

How I did it

Add enter and exit method to ConfigDBConnector

Work item tracking
  • Microsoft ADO: 24222755

How to verify it

Pass all UT and E2E test cases.
Add new UT to check ConfigDBConnector support 'with' statement

Which release branch to backport (provide reason below if selected)

  • 201811
  • 201911
  • 202006
  • 202012
  • 202106
  • 202111

Description for the changelog

Add support of 'with' statement to ConfigDBConnector

Link to config_db schema for YANG module changes

A picture of a cute animal (not mandatory but encouraged)

@liuh-80 liuh-80 changed the title [POC] Add support of 'with' statement to ConfigDBConnector Add support of 'with' statement to ConfigDBConnector Dec 4, 2023
@liuh-80 liuh-80 marked this pull request as ready for review December 4, 2023 09:25
@liuh-80 liuh-80 requested a review from qiluo-msft December 4, 2023 09:25
@liuh-80 liuh-80 force-pushed the dev/liuh/support_with_statement branch from 117fbdd to b998b56 Compare December 5, 2023 02:36
@@ -32,6 +32,11 @@ void DBInterface::close(const std::string& dbName)
m_redisClient.erase(dbName);
Copy link
Contributor

@qiluo-msft qiluo-msft Jan 9, 2024

Choose a reason for hiding this comment

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

erase

This erase does not release the resource immediately. We need to recusively close these objects, which may need new implementation in other classes. #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The 'with' state will not call this method, it will call the map::clear(), and because m_redisClient not store pointer or reference, the dtor of RedisContext will be invoke by clear() and release redis connection:

RedisContext::~RedisContext()
{
if(m_conn)
redisFree(m_conn);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree. The original comment is invalid.

@@ -32,6 +32,11 @@ void DBInterface::close(const std::string& dbName)
m_redisClient.erase(dbName);
}

void DBInterface::close_all()
Copy link
Contributor

@qiluo-msft qiluo-msft Jan 9, 2024

Choose a reason for hiding this comment

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

close_all

Is it possible to overload close function? I am not sure how SWIG handle the overload or reusing the same function with default parameter None easily.
#Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@liuh-80 liuh-80 merged commit ad4d386 into sonic-net:master Jan 19, 2024
14 checks passed
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.

2 participants