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

[SCB-1550] add a java client for ServiceComb-Kie #1366

Merged
merged 5 commits into from Nov 7, 2019

Conversation

zaneChou1
Copy link
Contributor

Add a sample Java client of ServiceComb-Kie to ServiceComb project ( ServiceComb-Java-chassis ).
Reference go-client ( https://github.com/apache/servicecomb-kie/blob/master/client/client.go)
Implement the Java client of ServiceComb-Kie.

@coveralls
Copy link

coveralls commented Oct 28, 2019

Coverage Status

Coverage decreased (-0.1%) to 85.724% when pulling cdd4251 on zaneChou1:uploadKieClient into 3896be3 on apache:master.

<parent>
<groupId>org.apache.servicecomb</groupId>
<artifactId>java-chassis-parent</artifactId>
<version>1.3.0-SNAPSHOT</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

Lastest has changed to 2.0.0-SNAPSHOT

Copy link
Contributor

Choose a reason for hiding this comment

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

  <parent>
    <groupId>org.apache.servicecomb</groupId>
    <artifactId>java-chassis-parent</artifactId>
    <version>2.0.0-SNAPSHOT</version>
    <relativePath>../parents/default</relativePath>
  </parent>

relativePath is required for compiling.

<dependency>
<groupId>org.apache.httpcomponents</groupId>
<artifactId>httpclient</artifactId>
<version>4.5.7</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

3td parties should managed by dependency management. see java-chassis-dependencies and take a look at other modules.

LOGGER.info("putKeyValue result:" + response.getContent());
return response.getContent();
} else {
throw new OperationsException(response.getStatusCode() + response.getMessage() + response.getContent());
Copy link
Contributor

Choose a reason for hiding this comment

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

Because you have catched the exception, this statement equals log the error.

throw new OperationsException(response.getStatusCode() + response.getMessage() + response.getContent());
}
} catch (IOException | OperationsException e) {
e.printStackTrace();
Copy link
Contributor

Choose a reason for hiding this comment

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

use Logger to log the message

try {
HttpResponse response = httpClient.getHttpRequest("/kie/kv/" + key, null, null);
if (response.getStatusCode() == HttpStatus.SC_OK) {
LOGGER.info("getKeyValue result:" + response.getContent());
Copy link
Contributor

Choose a reason for hiding this comment

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

Do not log the result of successful operation. Will cause a lot of unneccessary logs.

* @param key
* @return
*/
public List<KVResponse> getValueOfKey(String key) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this operation will not throw exception, should document this API that when return null, it is some error happens. And maybe define some XXXException for this module is better for users to use this API.


private String key;

private String label_id;
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these name required? Can make them to follow java conventions?

}

public HttpTransportImpl() {
httpClient = HttpClients.createDefault();
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to create a task to implement TLS. A lot of security settings should be set.

@@ -125,6 +125,7 @@
<module>java-chassis-spring-boot</module>
<module>inspector</module>
<module>solutions</module>
<module>client/kie-client</module>
Copy link
Contributor

Choose a reason for hiding this comment

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

Create a subtask to implement dynamic config module for kie.

@liubao68
Copy link
Contributor

liubao68 commented Nov 4, 2019

https://docs.servicecomb.io/java-chassis/zh_CN/build-consumer/3rd-party-service-invoke.html
Can you refer to document and anaylyse if we can use this feature to implement this client. Because this will add a lot of governance features for invocation, like flow control, metrics, tracing, etc. (not required, just for discussion)

@zaneChou1
Copy link
Contributor Author

@liubao68 Thank you for your comments. I optimized the code based on your suggestion.
ServiceComb-Kie does not provide TLS documentation and samples now. So the client not yet done support now and it can be increased later.
About "Create a subtask to implement dynamic config module for kie", the kie client can support dynamic config later, but it should be handled independently in the client directory.
How about you?

@liubao68
Copy link
Contributor

liubao68 commented Nov 4, 2019

Maybe your IDE formatter is wrong. Please use formatters in etc folder of this project to format code.

@zaneChou1
Copy link
Contributor Author

@liubao68 thanks for reminding, the code format is now standardized.

}

public Builder setDomainName(String domainName) {
if (domainName == null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think delete this code is better. Port, Host is the same. [minor]

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 domainName means projectName, mayebe this naming is easy to misunderstand. I can change the name to projectName.
The base api path is "/v1/{project}/kie/kv/" in swagger. The default value of {project} is default, which can also be customized by the user.

@liubao68 liubao68 merged commit 4577b42 into apache:master Nov 7, 2019
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.

None yet

3 participants