Skip to content

#692: Added get_metric_addr to the ClipperConnection#693

Merged
simon-mo merged 3 commits intoucbrise:developfrom
rkooo567:add_get_metric_addr_api
May 14, 2019
Merged

#692: Added get_metric_addr to the ClipperConnection#693
simon-mo merged 3 commits intoucbrise:developfrom
rkooo567:add_get_metric_addr_api

Conversation

@rkooo567
Copy link
Collaborator

Related to #692

Summary

Currently ClipperConnection API does not include get_admin_addr() and get_metric_addr(). I added these two functions to ClipperConnection.

@simon-mo I believe our CI should automatically update our API docs once it is merged, but let me know if there are other steps to do to update API documentation. (http://docs.clipper.ai/en/v0.3.0/)

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Clipper-PRB/1955/
Test FAILed.

@withsmilo
Copy link
Collaborator

@rkooo567
Good for quick PR. I have a question. Do we need to expose get_admin_addr() directly to users? The ClipperConnection in clipper_admin has a role of helping get_admin_addr() to use easily. How about using get_admin_addr() only for internal use?

@rkooo567
Copy link
Collaborator Author

@withsmilo You are right. I will remove get_admin_addr() Thanks for the feedback!

…nterface to communicate with management frontend. It is unncessary
Copy link
Collaborator

@withsmilo withsmilo left a comment

Choose a reason for hiding this comment

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

LGTM!

@withsmilo withsmilo changed the title #692: Added get_admin_addr and get_metric_addr to the ClipperConnecti… #692: Added get_metric_addr to the ClipperConnection May 14, 2019
@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Clipper-PRB/1956/
Test PASSed.

@withsmilo
Copy link
Collaborator

After reviewed by @simon-mo or @RehanSD , I will merge it.

@rkooo567
Copy link
Collaborator Author

@withsmilo Sounds good! Thanks a lot for checking it out

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