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

#692: Added get_metric_addr to the ClipperConnection #693

Merged
merged 3 commits into from May 14, 2019
Merged

#692: Added get_metric_addr to the ClipperConnection #693

merged 3 commits into from May 14, 2019

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/)

@rkooo567 rkooo567 self-assigned this May 13, 2019
@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!

@rkooo567
ucbrise#692: removed get_admin_addr because clipperConnection is an i…
d752a78 
…nterface to communicate with management frontend. It is unncessary
withsmilo
withsmilo approved these changes May 14, 2019
View reviewed changes
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

@simon-mo simon-mo merged commit 6e6c0c7 into ucbrise:develop May 14, 2019
@jacekwachowiak jacekwachowiak mentioned this pull request May 15, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@withsmilo withsmilo withsmilo approved these changes

@simon-mo simon-mo Awaiting requested review from simon-mo

@RehanSD RehanSD Awaiting requested review from RehanSD

Assignees

@rkooo567 rkooo567

Labels
status: accepted type: enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@rkooo567 @AmplabJenkins @withsmilo @simon-mo