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

[Enhancement]: RestProtocol #3480

Merged
merged 2 commits into from
Feb 17, 2019
+20 −30
Merged

[Enhancement]: RestProtocol #3480

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d39e3e8
optimization: RestProtocol
kezhenxu94 Feb 14, 2019
7131e86
update as requested
kezhenxu94 Feb 16, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 20 additions & 30 deletions dubbo-rpc/dubbo-rpc-rest/src/main/java/org/apache/dubbo/rpc/protocol/rest/RestProtocol.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,16 +28,13 @@

import org.apache.http.HeaderElement;
import org.apache.http.HeaderElementIterator;
import org.apache.http.HttpResponse;
import org.apache.http.client.config.RequestConfig;
import org.apache.http.config.SocketConfig;
import org.apache.http.conn.ConnectionKeepAliveStrategy;
import org.apache.http.impl.client.CloseableHttpClient;
import org.apache.http.impl.client.HttpClientBuilder;
import org.apache.http.impl.conn.PoolingHttpClientConnectionManager;
import org.apache.http.message.BasicHeaderElementIterator;
import org.apache.http.protocol.HTTP;
import org.apache.http.protocol.HttpContext;
import org.jboss.resteasy.client.jaxrs.ResteasyClient;
import org.jboss.resteasy.client.jaxrs.ResteasyClientBuilder;
import org.jboss.resteasy.client.jaxrs.ResteasyWebTarget;
Expand Down Expand Up @@ -65,7 +62,7 @@ public class RestProtocol extends AbstractProxyProtocol {
private static final int HTTPCLIENTCONNECTIONMANAGER_CLOSEWAITTIME_MS = 1000;
private static final int HTTPCLIENTCONNECTIONMANAGER_CLOSEIDLETIME_S = 30;

private final Map<String, RestServer> servers = new ConcurrentHashMap<String, RestServer>();
private final Map<String, RestServer> servers = new ConcurrentHashMap<>();

private final RestServerFactory serverFactory = new RestServerFactory();

Expand All @@ -91,12 +88,11 @@ public int getDefaultPort() {
protected <T> Runnable doExport(T impl, Class<T> type, URL url) throws RpcException {
String addr = getAddr(url);
Class implClass = ApplicationModel.getProviderModel(url.getServiceKey()).getServiceInstance().getClass();
RestServer server = servers.get(addr);
if (server == null) {
server = serverFactory.createServer(url.getParameter(Constants.SERVER_KEY, DEFAULT_SERVER));
server.start(url);
servers.put(addr, server);
}
RestServer server = servers.computeIfAbsent(addr, addr0 -> {
Copy link
Member

Choose a reason for hiding this comment

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

It is recommended to change addr0 to restServer

Copy link
Member Author

Choose a reason for hiding this comment

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

why？？

Copy link
Member Author

Choose a reason for hiding this comment

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

@CrazyHZM please refer to the JavaDoc of computeIfAbsent, note the second parameter.

Copy link
Member

Choose a reason for hiding this comment

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

Not a grammatical error, just add readability

Copy link
Contributor

Choose a reason for hiding this comment

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

I also think we should replace addr0 into restServer. the addr0 is not clear for people to understand.

Copy link
Member Author

Choose a reason for hiding this comment

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

but I'm wondering why renaming a variable of type String to restServer increases readability

Copy link
Member

Choose a reason for hiding this comment

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

This has nothing to do with whether it is a String type, but using restServer in the meaning description will be clearer than addr0.
This is also to let others see the source code, know that the second parameter of the servers is a RestServer type.

Copy link
Member Author

@kezhenxu94 kezhenxu94 Feb 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@scxwhite @CrazyHZM Since the changes is controversial, I've reverted the changes, as for

This is also to let others see the source code, know that the second parameter of the servers is a RestServer type.

I'd rather suggest renaming the servers to something like serversKeyedByAddress or anything else but what you said.

Copy link
Member

Choose a reason for hiding this comment

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

That part of the code does not need to reverted, I think you modified is good, just change addr0 to restServer

Copy link
Member Author

Choose a reason for hiding this comment

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

Changes are made as requested

RestServer s = serverFactory.createServer(url.getParameter(Constants.SERVER_KEY, DEFAULT_SERVER));
s.start(url);
return s;
});

String contextPath = getContextPath(url);
if ("servlet".equalsIgnoreCase(url.getParameter(Constants.SERVER_KEY, DEFAULT_SERVER))) {
Expand Down Expand Up @@ -124,13 +120,10 @@ protected <T> Runnable doExport(T impl, Class<T> type, URL url) throws RpcExcept
server.deploy(resourceDef, impl, contextPath);

final RestServer s = server;
return new Runnable() {
@Override
public void run() {
// TODO due to dubbo's current architecture,
// it will be called from registry protocol in the shutdown process and won't appear in logs
s.undeploy(resourceDef);
}
return () -> {
// TODO due to dubbo's current architecture,
// it will be called from registry protocol in the shutdown process and won't appear in logs
s.undeploy(resourceDef);
};
}

Expand Down Expand Up @@ -159,20 +152,17 @@ protected <T> T doRefer(Class<T> serviceType, URL url) throws RpcException {
.build();

CloseableHttpClient httpClient = HttpClientBuilder.create()
.setKeepAliveStrategy(new ConnectionKeepAliveStrategy() {
@Override
public long getKeepAliveDuration(HttpResponse response, HttpContext context) {
HeaderElementIterator it = new BasicHeaderElementIterator(response.headerIterator(HTTP.CONN_KEEP_ALIVE));
while (it.hasNext()) {
HeaderElement he = it.nextElement();
String param = he.getName();
String value = he.getValue();
if (value != null && param.equalsIgnoreCase(Constants.TIMEOUT_KEY)) {
return Long.parseLong(value) * 1000;
}
.setKeepAliveStrategy((response, context) -> {
HeaderElementIterator it = new BasicHeaderElementIterator(response.headerIterator(HTTP.CONN_KEEP_ALIVE));
while (it.hasNext()) {
HeaderElement he = it.nextElement();
String param = he.getName();
String value = he.getValue();
if (value != null && param.equalsIgnoreCase(Constants.TIMEOUT_KEY)) {
return Long.parseLong(value) * 1000;
}
return HTTPCLIENT_KEEPALIVEDURATION;
}
return HTTPCLIENT_KEEPALIVEDURATION;
})
.setDefaultRequestConfig(requestConfig)
.setDefaultSocketConfig(socketConfig)
Expand Down Expand Up @@ -245,7 +235,7 @@ protected String getContextPath(URL url) {

protected class ConnectionMonitor extends Thread {
private volatile boolean shutdown;
private final List<PoolingHttpClientConnectionManager> connectionManagers = Collections.synchronizedList(new LinkedList<PoolingHttpClientConnectionManager>());
private final List<PoolingHttpClientConnectionManager> connectionManagers = Collections.synchronizedList(new LinkedList<>());

public void addConnectionManager(PoolingHttpClientConnectionManager connectionManager) {
connectionManagers.add(connectionManager);
Expand Down