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

Removed DB specific get api's from Selectable class #378

Merged
merged 3 commits into from
Sep 2, 2020

Conversation

abdosi
Copy link
Contributor

@abdosi abdosi commented Sep 1, 2020

Why/What I did:-

Change 1:
As per review comments on #376
removed DB specific API from selectable class
to keep it generic.

Change 2:
However to access to the Dervied Class Redis Select
DB API's via python we need to downcast the the Selectable Object
to RedisSelect object.
To do this added Helper function in swig .i file Ref: http://www.swig.org/Doc3.0/Python.html

How I verify:
After this change updated client caclmgrd to call new downcast API and verified we are getting correct information.
Sample Client Side code after this change -

    (state, selectableObj) = sel.select(SELECT_TIMEOUT_MS)
        # Continue if select is timeout or selectable object is not return
        if state != swsscommon.Select.OBJECT:
            continue
        # Get the corresponding namespace from selectable object

        redisSelectObj = swsscommon.CastSelectableToRedisSelectObj(selectableObj)

        namespace = redisSelectObj.getDbConnector().getNamespace()

sonic-net#376
to remove DB specific API from selectable class
which is generic.

However to access to the Derviced Class Redis Select
API via python we need to downcast the the Selectable Object.

Added Helpfer function to do same.

Ref: http://www.swig.org/Doc3.0/Python.html

Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>
@abdosi abdosi changed the title Remove DB specifig get api Removed DB specific get api's from Selectable class Sep 1, 2020
Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>
int getDbConnectorId() override;
std::string getDbNamespace() override;
int getDbConnectorId();
std::string getDbNamespace();
Copy link
Contributor

@qiluo-msft qiluo-msft Sep 1, 2020

Choose a reason for hiding this comment

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

Suggest add only one function const DBConnector* getDbConnector() const, and caller could call ret->getNamespace(), or any other member functions. #Closed

Copy link
Contributor Author

@abdosi abdosi Sep 2, 2020

Choose a reason for hiding this comment

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

Since the DBConnector is unique_ptr return the
reference to the contained object, or a non owning pointer

Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>
@abdosi
Copy link
Contributor Author

abdosi commented Sep 2, 2020

@judyjoseph Please update sonic-py-common client accordingly.

@abdosi abdosi merged commit 200f2b0 into sonic-net:master Sep 2, 2020
@abdosi abdosi deleted the select_ns_cleanup branch September 2, 2020 18:37
@judyjoseph
Copy link
Collaborator

@judyjoseph Please update sonic-py-common client accordingly.

sure will take care of updating the pmon application ..Thanks for this change !

abdosi added a commit that referenced this pull request Sep 3, 2020
* Address Review comments
#376
to remove DB specific API from selectable class
which is generic.

However to access to the Derviced Class Redis Select
API via python we need to downcast the the Selectable Object.

Added Helpfer function to do same.

Ref: http://www.swig.org/Doc3.0/Python.html

Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>

* Align the code properly

Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>

* Address Review Comments. Return the DBConnector Pointer.
Since the DBConnector is unique_ptr return the
reference to the contained object, or a non owning pointer

Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>
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.

3 participants