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

code optimization #3118

Merged
merged 4 commits into from
Jan 4, 2019
Merged

code optimization #3118

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bb7932d
code optimization
CrazyHZM Jan 3, 2019
e4ef2c7
useless import
CrazyHZM Jan 3, 2019
3f9b4e2
Merge branch 'master' into codeOptimization
CrazyHZM Jan 3, 2019
802ad54
optimization
CrazyHZM Jan 3, 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
4 changes: 2 additions & 2 deletions dubbo-cluster/src/main/java/org/apache/dubbo/rpc/cluster/directory/StaticDirectory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@

/**
* StaticDirectory
*
*/
public class StaticDirectory<T> extends AbstractDirectory<T> {
private static final Logger logger = LoggerFactory.getLogger(StaticDirectory.class);
Expand All @@ -50,8 +49,9 @@ public StaticDirectory(URL url, List<Invoker<T>> invokers) {

public StaticDirectory(URL url, List<Invoker<T>> invokers, RouterChain<T> routerChain) {
super(url == null && invokers != null && !invokers.isEmpty() ? invokers.get(0).getUrl() : url, routerChain);
if (invokers == null || invokers.isEmpty())
if (invokers == null || invokers.isEmpty()) {
ralf0131 marked this conversation as resolved.
Show resolved Hide resolved
throw new IllegalArgumentException("invokers == null");
}
this.invokers = invokers;
}

Expand Down
1 change: 1 addition & 0 deletions dubbo-cluster/src/main/java/org/apache/dubbo/rpc/cluster/router/AbstractRouter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ public int compareTo(Router o) {
return (this.getPriority() >= o.getPriority()) ? 1 : -1;
}

@Override
public int getPriority() {
return priority;
}
Expand Down
40 changes: 20 additions & 20 deletions dubbo-common/src/main/java/org/apache/dubbo/common/logger/Logger.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,144 +28,144 @@ public interface Logger {
*
* @param msg log this message
*/
public void trace(String msg);
void trace(String msg);

/**
* Logs an error with trace log level.
*
* @param e log this cause
*/
public void trace(Throwable e);
void trace(Throwable e);

/**
* Logs an error with trace log level.
*
* @param msg log this message
* @param e log this cause
*/
public void trace(String msg, Throwable e);
void trace(String msg, Throwable e);

/**
* Logs a message with debug log level.
*
* @param msg log this message
*/
public void debug(String msg);
void debug(String msg);

/**
* Logs an error with debug log level.
*
* @param e log this cause
*/
public void debug(Throwable e);
void debug(Throwable e);

/**
* Logs an error with debug log level.
*
* @param msg log this message
* @param e log this cause
*/
public void debug(String msg, Throwable e);
void debug(String msg, Throwable e);

/**
* Logs a message with info log level.
*
* @param msg log this message
*/
public void info(String msg);
void info(String msg);

/**
* Logs an error with info log level.
*
* @param e log this cause
*/
public void info(Throwable e);
void info(Throwable e);

/**
* Logs an error with info log level.
*
* @param msg log this message
* @param e log this cause
*/
public void info(String msg, Throwable e);
void info(String msg, Throwable e);

/**
* Logs a message with warn log level.
*
* @param msg log this message
*/
public void warn(String msg);
void warn(String msg);

/**
* Logs a message with warn log level.
*
* @param e log this message
*/
public void warn(Throwable e);
void warn(Throwable e);

/**
* Logs a message with warn log level.
*
* @param msg log this message
* @param e log this cause
*/
public void warn(String msg, Throwable e);
void warn(String msg, Throwable e);

/**
* Logs a message with error log level.
*
* @param msg log this message
*/
public void error(String msg);
void error(String msg);

/**
* Logs an error with error log level.
*
* @param e log this cause
*/
public void error(Throwable e);
void error(Throwable e);

/**
* Logs an error with error log level.
*
* @param msg log this message
* @param e log this cause
*/
public void error(String msg, Throwable e);
void error(String msg, Throwable e);

/**
* Is trace logging currently enabled?
*
* @return true if trace is enabled
*/
public boolean isTraceEnabled();
boolean isTraceEnabled();

/**
* Is debug logging currently enabled?
*
* @return true if debug is enabled
*/
public boolean isDebugEnabled();
boolean isDebugEnabled();

/**
* Is info logging currently enabled?
*
* @return true if info is enabled
*/
public boolean isInfoEnabled();
boolean isInfoEnabled();

/**
* Is warn logging currently enabled?
*
* @return true if warn is enabled
*/
public boolean isWarnEnabled();
boolean isWarnEnabled();

/**
* Is error logging currently enabled?
*
* @return true if error is enabled
*/
public boolean isErrorEnabled();
boolean isErrorEnabled();

}
29 changes: 15 additions & 14 deletions dubbo-common/src/main/java/org/apache/dubbo/common/utils/NetUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
*/
package org.apache.dubbo.common.utils;

import org.apache.dubbo.common.Constants;
import org.apache.dubbo.common.URL;
import org.apache.dubbo.common.logger.Logger;
import org.apache.dubbo.common.logger.LoggerFactory;
Expand All @@ -37,8 +38,6 @@
*/
public class NetUtils {

public static final String LOCALHOST = "127.0.0.1";
public static final String ANYHOST = "0.0.0.0";
private static final Logger logger = LoggerFactory.getLogger(NetUtils.class);
private static final int RND_PORT_START = 30000;

Expand Down Expand Up @@ -109,18 +108,18 @@ public static boolean isValidAddress(String address) {
public static boolean isLocalHost(String host) {
return host != null
&& (LOCAL_IP_PATTERN.matcher(host).matches()
|| host.equalsIgnoreCase("localhost"));
|| host.equalsIgnoreCase(Constants.LOCALHOST_KEY));
}

public static boolean isAnyHost(String host) {
return "0.0.0.0".equals(host);
return Constants.ANYHOST_VALUE.equals(host);
}

public static boolean isInvalidLocalHost(String host) {
return host == null
|| host.length() == 0
|| host.equalsIgnoreCase("localhost")
|| host.equals("0.0.0.0")
|| host.equalsIgnoreCase(Constants.LOCALHOST_KEY)
|| host.equals(Constants.ANYHOST_VALUE)
|| (LOCAL_IP_PATTERN.matcher(host).matches());
}

Expand All @@ -139,13 +138,14 @@ static boolean isValidAddress(InetAddress address) {
}
String name = address.getHostAddress();
return (name != null
&& !ANYHOST.equals(name)
&& !LOCALHOST.equals(name)
&& !Constants.ANYHOST_VALUE.equals(name)
&& !Constants.LOCALHOST_VALUE.equals(name)
&& IP_PATTERN.matcher(name).matches());
Copy link
Contributor

Choose a reason for hiding this comment

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

@CrazyHZM
I think once the name is not null the rest of two evaulation

!Constants.ANYHOST_VALUE.equals(name)
                && !Constants.LOCALHOST_VALUE.equals(name)

most likely be evaluates to true for non localhost so the chances of this whole condition to become true depends on IP_PATTERN.matcher(name).matches(). So from better understanding and performance point of view I think it would be better if we change it to

(name != null
                && IP_PATTERN.matcher(name).matches()
                && !Constants.ANYHOST_VALUE.equals(name)
                && !Constants.LOCALHOST_VALUE.equals(name));

What do yu say?

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks, I think this idea is good. I will modify it.

}

/**
* Check if an ipv6 address is reachable.
*
* @param address the given address
* @return true if it is reachable
*/
Expand All @@ -166,12 +166,13 @@ static boolean isValidV6Address(Inet6Address address) {
* normalize the ipv6 Address, convert scope name to scope id.
* e.g.
* convert
* fe80:0:0:0:894:aeec:f37d:23e1%en0
* fe80:0:0:0:894:aeec:f37d:23e1%en0
* to
* fe80:0:0:0:894:aeec:f37d:23e1%5
*
* fe80:0:0:0:894:aeec:f37d:23e1%5
* <p>
* The %5 after ipv6 address is called scope id.
* see java doc of {@link Inet6Address} for more details.
*
* @param address the input address
* @return the normalized address, with scope id converted to int
*/
Expand All @@ -191,7 +192,7 @@ static InetAddress normalizeV6Address(Inet6Address address) {

public static String getLocalHost() {
InetAddress address = getLocalAddress();
return address == null ? LOCALHOST : address.getHostAddress();
return address == null ? Constants.LOCALHOST_VALUE : address.getHostAddress();
Copy link
Contributor

@khanimteyaz khanimteyaz Jan 3, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I think Constants.LOCALHOST should be good and Constants.LOCALHOST_VALUE looks like extra here. What do you say?

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks,I am not sure about this, I feel no difference.

Copy link
Contributor

Choose a reason for hiding this comment

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

LOCALHOST is good enough, LOCALHOST_VALUE sound extra here. Having said this, it is very minor both should be ok. I leave it to you. If any has any suggestion it can be modified ltr. :)

}

public static String filterLocalHost(String host) {
Expand Down Expand Up @@ -236,7 +237,7 @@ private static InetAddress getLocalAddress0() {
localAddress = InetAddress.getLocalHost();
if (localAddress instanceof Inet6Address) {
Inet6Address address = (Inet6Address) localAddress;
if (isValidV6Address(address)){
if (isValidV6Address(address)) {
return normalizeV6Address(address);
}
} else if (isValidAddress(localAddress)) {
Expand All @@ -259,7 +260,7 @@ private static InetAddress getLocalAddress0() {
InetAddress address = addresses.nextElement();
if (address instanceof Inet6Address) {
Inet6Address v6Address = (Inet6Address) address;
if (isValidV6Address(v6Address)){
if (isValidV6Address(v6Address)) {
return normalizeV6Address(v6Address);
}
} else if (isValidAddress(address)) {
Expand Down
8 changes: 4 additions & 4 deletions dubbo-config/dubbo-config-api/src/main/java/org/apache/dubbo/config/AbstractConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,9 +318,9 @@ protected static void checkExtension(Class<?> type, String property, String valu
* Check whether there is a <code>Extension</code> who's name (property) is <code>value</code> (special treatment is
* required)
*
* @param type The Extension type
* @param type The Extension type
* @param property The extension key
* @param value The Extension name
* @param value The Extension name
*/
protected static void checkMultiExtension(Class<?> type, String property, String value) {
checkMultiName(property, value);
Expand Down Expand Up @@ -500,7 +500,7 @@ public Map<String, String> getMetaData() {
try {
String name = method.getName();
if ((name.startsWith("get") || name.startsWith("is"))
&& !name.equals("get")
&& !"get".equals(name)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would preffer to extract out this to a boolean method with meaningfull name.
e.g.

if (isMetaMethod(method)) {
}

private boolean isMetaMethod(Method method) {
   String name=method.getName();
   if(!((name.startsWith("get") || name.startsWith("is"))) {
         return false;
   }
    if( !"get".equals(name) && !"getClass".equals(name)) {
        return false;
    }
   if(!Modifier.isPublic(method.getModifiers()) {
      return false;
   }
    if( method.getParameterTypes().length > 0 ) {
       return false;
    }
   if(!ClassHelper.isPrimitive(method.getReturnType())) {
      return false;
   }
   return true;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks, I think this idea is good. I will modify it.

Copy link
Member Author

Choose a reason for hiding this comment

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

if( !"get".equals(name) && !"getClass".equals(name)) { return false; }
Here is wrong.
I have modified it elsewhere, you can re-examine it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool.

&& !"getClass".equals(name)
&& Modifier.isPublic(method.getModifiers())
&& method.getParameterTypes().length == 0
Expand Down Expand Up @@ -565,7 +565,7 @@ public void refresh() {
config.addProperties(getMetaData());
if (Environment.getInstance().isConfigCenterFirst()) {
// The sequence would be: SystemConfiguration -> ExternalConfiguration -> AppExternalConfiguration -> AbstractConfig -> PropertiesConfiguration
compositeConfiguration.addConfiguration(3,config);
compositeConfiguration.addConfiguration(3, config);
} else {
// The sequence would be: SystemConfiguration -> AbstractConfig -> ExternalConfiguration -> AppExternalConfiguration -> PropertiesConfiguration
compositeConfiguration.addConfiguration(1, config);
Expand Down
4 changes: 2 additions & 2 deletions dubbo-config/dubbo-config-api/src/main/java/org/apache/dubbo/config/ReferenceConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ public class ReferenceConfig<T> extends AbstractReferenceConfig {
*
* <li>when the url is dubbo://224.5.6.7:1234/org.apache.dubbo.config.api.DemoService?application=dubbo-sample, then
* the protocol is <b>DubboProtocol</b></li>
*
* <p>
* Actually，when the {@link ExtensionLoader} init the {@link Protocol} instants,it will automatically wraps two
* layers, and eventually will get a <b>ProtocolFilterWrapper</b> or <b>ProtocolListenerWrapper</b>
*/
Expand Down Expand Up @@ -327,7 +327,7 @@ private T createProxy(Map<String, String> map) {
}

if (isJvmRefer) {
URL url = new URL(Constants.LOCAL_PROTOCOL, NetUtils.LOCALHOST, 0, interfaceClass.getName()).addParameters(map);
URL url = new URL(Constants.LOCAL_PROTOCOL, Constants.LOCALHOST_VALUE, 0, interfaceClass.getName()).addParameters(map);
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above.

invoker = refprotocol.refer(interfaceClass, url);
if (logger.isInfoEnabled()) {
logger.info("Using injvm service " + interfaceClass.getName());
Expand Down
6 changes: 3 additions & 3 deletions dubbo-config/dubbo-config-api/src/main/java/org/apache/dubbo/config/ServiceConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.TimeUnit;

import static org.apache.dubbo.common.utils.NetUtils.LOCALHOST;
import static org.apache.dubbo.common.Constants.LOCALHOST_VALUE;
import static org.apache.dubbo.common.utils.NetUtils.getAvailablePort;
import static org.apache.dubbo.common.utils.NetUtils.getLocalHost;
import static org.apache.dubbo.common.utils.NetUtils.isInvalidLocalHost;
Expand All @@ -82,7 +82,7 @@ public class ServiceConfig<T> extends AbstractServiceConfig {
*
* <li>when the url is dubbo://224.5.6.7:1234/org.apache.dubbo.config.api.DemoService?application=dubbo-sample, then
* the protocol is <b>DubboProtocol</b></li>
*
* <p>
* Actually，when the {@link ExtensionLoader} init the {@link Protocol} instants,it will automatically wraps two
* layers, and eventually will get a <b>ProtocolFilterWrapper</b> or <b>ProtocolListenerWrapper</b>
*/
Expand Down Expand Up @@ -583,7 +583,7 @@ private void exportLocal(URL url) {
if (!Constants.LOCAL_PROTOCOL.equalsIgnoreCase(url.getProtocol())) {
URL local = URL.valueOf(url.toFullString())
.setProtocol(Constants.LOCAL_PROTOCOL)
.setHost(LOCALHOST)
.setHost(LOCALHOST_VALUE)
.setPort(0);
Exporter<?> exporter = protocol.export(
proxyFactory.getInvoker(ref, (Class) interfaceClass, local));
Expand Down
8 changes: 6 additions & 2 deletions ...definition/src/main/java/org/apache/dubbo/metadata/definition/model/MethodDefinition.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,12 @@ public String toString() {

@Override
public boolean equals(Object o) {
if (this == o) return true;
if (!(o instanceof MethodDefinition)) return false;
if (this == o) {
return true;
}
if (!(o instanceof MethodDefinition)) {
return false;
}
MethodDefinition that = (MethodDefinition) o;
return Objects.equals(getName(), that.getName()) &&
Arrays.equals(getParameterTypes(), that.getParameterTypes()) &&
Expand Down
8 changes: 6 additions & 2 deletions ...efinition/src/main/java/org/apache/dubbo/metadata/definition/model/ServiceDefinition.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,8 +80,12 @@ public String toString() {

@Override
public boolean equals(Object o) {
if (this == o) return true;
if (!(o instanceof ServiceDefinition)) return false;
if (this == o) {
return true;
}
if (!(o instanceof ServiceDefinition)) {
return false;
}
ServiceDefinition that = (ServiceDefinition) o;
return Objects.equals(getCanonicalName(), that.getCanonicalName()) &&
Objects.equals(getCodeSource(), that.getCodeSource()) &&
Expand Down
Loading