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 OpenSergoClientManager to support client reuse #24

Merged
merged 1 commit into from
Feb 7, 2023

Conversation

jnan806
Copy link
Collaborator

@jnan806 jnan806 commented Feb 6, 2023

Signed-off-by: Jiangnan Jia [email protected]

Describe what this PR does / why we need it

support client reuse

Does this pull request fix one issue?

Fixes #8

Describe how you did it

  • Add OpenSergoClientManager to manage the reuse OpenSergoClient
  • Add Builder in OpenSergoClient for creating instance
  • Add OpenSergoConfig to support customized config

Describe how to verify it

  • for shared OpenSergoClient
OpenSergoClient openSergoClient = OpenSergoClientManager.get().getOrCreateClient("127.0.0.1", 10246, new OpenSergoConfig());
  • for prototype OpenSergoClient
OpenSergoClient openSergoClient = new OpenSergoClient.Builder().endpoint("127.0.0.1", 
10246).openSergoConfig(new OpenSergoConfig()).build();

Special notes for reviews

@jnan806 jnan806 requested a review from sczyh30 February 7, 2023 00:51
* @param config OpenSergoConfig
* @return OpenSergoClient
*/
public OpenSergoClient getOrCreateClient(String host, int port, OpenSergoConfig config) {
Copy link
Member

Choose a reason for hiding this comment

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

Pay attention to thread-safety here.

return this;
}

public OpenSergoClient.Builder openSergoConfig(OpenSergoConfig openSergoConfig) {
Copy link
Member

Choose a reason for hiding this comment

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

后面我们需要优化 builder 里面的元素,是不是可以把 OpenSergoConfig 里面的配置平铺到 builder 里面处理,让用户更易用?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

是把 OpenSergoConfig 取消掉吗?

Copy link
Member

@sczyh30 sczyh30 left a comment

Choose a reason for hiding this comment

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

LGTM. I'll improve the details later.

@sczyh30 sczyh30 merged commit 0a161d9 into opensergo:main Feb 7, 2023
@sczyh30
Copy link
Member

sczyh30 commented Feb 7, 2023

Thanks!

@sczyh30 sczyh30 added the kind/feature Category issues or PRs related to feature request label Feb 7, 2023
@jnan806 jnan806 deleted the clientmanager branch February 7, 2023 03:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Category issues or PRs related to feature request
Projects
None yet
2 participants