You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tubemq.apache.org by GitBox <gi...@apache.org> on 2020/05/25 08:11:49 UTC
[GitHub] [incubator-tubemq] viviel opened a new pull request #97: [TUBEMQ-141] Remove the requirement to provide localHostIP
viviel opened a new pull request #97:
URL: https://github.com/apache/incubator-tubemq/pull/97
1. Remove the requirement to provide localHostIP
2. Optimize the TubeClientConfig constructor to remove some explicit exceptions
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tubemq] viviel commented on a change in pull request #97: [TUBEMQ-141] Remove the requirement to provide localHostIP
Posted by GitBox <gi...@apache.org>.
viviel commented on a change in pull request #97:
URL: https://github.com/apache/incubator-tubemq/pull/97#discussion_r429901304
##########
File path: tubemq-client/src/main/java/org/apache/tubemq/client/config/TubeClientConfig.java
##########
@@ -92,23 +92,19 @@
// TLS configuration.
private TLSConfig tlsConfig = new TLSConfig();
+ public TubeClientConfig(String masterAddrInfo) {
+ this(new MasterInfo(masterAddrInfo));
+ }
- public TubeClientConfig(final String localHostIP, final String masterAddrInfo) throws Exception {
- if (TStringUtils.isBlank(localHostIP)) {
- throw new IllegalArgumentException("Illegal parameter: localHostIP is blank!");
- }
- // Not allow to set local host ip to 127.0.0.1.
- if ("127.0.0.1".equals(localHostIP)) {
- throw new IllegalArgumentException("Illegal parameter: localHostIP can't set to 127.0.0.1");
- }
- if (TStringUtils.isBlank(masterAddrInfo)) {
- throw new IllegalArgumentException("Illegal parameter: masterAddrInfo is Blank!");
- }
- this.masterInfo = new MasterInfo(masterAddrInfo);
- AddressUtils.validLocalIp(localHostIP.trim());
+ public TubeClientConfig(MasterInfo masterInfo) {
+ this(AddressUtils.getLocalIPv4Address(), masterInfo);
+ }
+
+ public TubeClientConfig(final String localHostIP, final String masterAddrInfo) {
+ this(localHostIP, new MasterInfo(masterAddrInfo));
}
- public TubeClientConfig(final String localHostIP, final MasterInfo masterInfo) throws Exception {
+ public TubeClientConfig(final String localHostIP, final MasterInfo masterInfo) {
Review comment:
If it is confirmed that the IP will be automatically and uniformly configured later, it is actually better. But will some people specify the IP by themselves or fill in the IP by specifying the network interface?This method is provided here:
```
AddressUtils.getIPV4AddressByNetworkInterfaceName("eth0");
```
I am not sure if there is such a scene
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tubemq] gosonzhang commented on a change in pull request #97: [TUBEMQ-141] Remove the requirement to provide localHostIP
Posted by GitBox <gi...@apache.org>.
gosonzhang commented on a change in pull request #97:
URL: https://github.com/apache/incubator-tubemq/pull/97#discussion_r429904551
##########
File path: tubemq-core/src/main/java/org/apache/tubemq/corebase/utils/AddressUtils.java
##########
@@ -130,4 +134,100 @@ public static String getRemoteAddressIP(Channel channel) {
}
return strRemoteIP;
}
+
+ public static String getLocalIPv4Address() {
+ List<String> ips = listNonLoopbackIPV4Address();
+
+ if (ips.isEmpty()) {
+ try {
+ return InetAddress.getLocalHost().getHostAddress();
+ } catch (UnknownHostException e) {
+ String errorMsg = "Unable to obtain local IP, please manually specify the local IP address";
+ throw new AddressException(errorMsg, e.getCause());
+ }
+ }
+
+ for (String ip : ips) {
+ if (ip.startsWith("192.168")) {
+ continue;
+ }
+
+ return ip;
+ }
+
+ return ips.get(ips.size() - 1);
Review comment:
If ips is empty, how to do?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tubemq] gosonzhang commented on a change in pull request #97: [TUBEMQ-141] Remove the requirement to provide localHostIP
Posted by GitBox <gi...@apache.org>.
gosonzhang commented on a change in pull request #97:
URL: https://github.com/apache/incubator-tubemq/pull/97#discussion_r431195106
##########
File path: tubemq-core/src/main/java/org/apache/tubemq/corebase/utils/AddressUtils.java
##########
@@ -34,31 +39,33 @@ public static synchronized String getLocalAddress() throws Exception {
return localIPAddress;
}
- public static boolean validLocalIp(String currLocalHost) throws Exception {
+ public static synchronized void setLocalAddress(String localIPAddress) {
+ AddressUtils.localIPAddress = localIPAddress;
+ }
+
+ public static boolean validLocalIp(String currLocalHost) {
Review comment:
The validLocalIp function seems to be superfluous and should be removed along with the sub-functions
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tubemq] gosonzhang commented on a change in pull request #97: [TUBEMQ-141] Remove the requirement to provide localHostIP
Posted by GitBox <gi...@apache.org>.
gosonzhang commented on a change in pull request #97:
URL: https://github.com/apache/incubator-tubemq/pull/97#discussion_r429896902
##########
File path: tubemq-client/src/main/java/org/apache/tubemq/client/config/TubeClientConfig.java
##########
@@ -92,23 +92,19 @@
// TLS configuration.
private TLSConfig tlsConfig = new TLSConfig();
+ public TubeClientConfig(String masterAddrInfo) {
+ this(new MasterInfo(masterAddrInfo));
+ }
- public TubeClientConfig(final String localHostIP, final String masterAddrInfo) throws Exception {
- if (TStringUtils.isBlank(localHostIP)) {
- throw new IllegalArgumentException("Illegal parameter: localHostIP is blank!");
- }
- // Not allow to set local host ip to 127.0.0.1.
- if ("127.0.0.1".equals(localHostIP)) {
- throw new IllegalArgumentException("Illegal parameter: localHostIP can't set to 127.0.0.1");
- }
- if (TStringUtils.isBlank(masterAddrInfo)) {
- throw new IllegalArgumentException("Illegal parameter: masterAddrInfo is Blank!");
- }
- this.masterInfo = new MasterInfo(masterAddrInfo);
- AddressUtils.validLocalIp(localHostIP.trim());
+ public TubeClientConfig(MasterInfo masterInfo) {
+ this(AddressUtils.getLocalIPv4Address(), masterInfo);
+ }
+
+ public TubeClientConfig(final String localHostIP, final String masterAddrInfo) {
Review comment:
need to add a @Deprecated tag on this function
##########
File path: tubemq-client/src/main/java/org/apache/tubemq/client/config/TubeClientConfig.java
##########
@@ -92,23 +92,19 @@
// TLS configuration.
private TLSConfig tlsConfig = new TLSConfig();
+ public TubeClientConfig(String masterAddrInfo) {
+ this(new MasterInfo(masterAddrInfo));
+ }
- public TubeClientConfig(final String localHostIP, final String masterAddrInfo) throws Exception {
- if (TStringUtils.isBlank(localHostIP)) {
- throw new IllegalArgumentException("Illegal parameter: localHostIP is blank!");
- }
- // Not allow to set local host ip to 127.0.0.1.
- if ("127.0.0.1".equals(localHostIP)) {
- throw new IllegalArgumentException("Illegal parameter: localHostIP can't set to 127.0.0.1");
- }
- if (TStringUtils.isBlank(masterAddrInfo)) {
- throw new IllegalArgumentException("Illegal parameter: masterAddrInfo is Blank!");
- }
- this.masterInfo = new MasterInfo(masterAddrInfo);
- AddressUtils.validLocalIp(localHostIP.trim());
+ public TubeClientConfig(MasterInfo masterInfo) {
+ this(AddressUtils.getLocalIPv4Address(), masterInfo);
+ }
+
+ public TubeClientConfig(final String localHostIP, final String masterAddrInfo) {
+ this(localHostIP, new MasterInfo(masterAddrInfo));
}
- public TubeClientConfig(final String localHostIP, final MasterInfo masterInfo) throws Exception {
+ public TubeClientConfig(final String localHostIP, final MasterInfo masterInfo) {
Review comment:
need to add a @Deprecated tag on this function
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tubemq] viviel commented on a change in pull request #97: [TUBEMQ-141] Remove the requirement to provide localHostIP
Posted by GitBox <gi...@apache.org>.
viviel commented on a change in pull request #97:
URL: https://github.com/apache/incubator-tubemq/pull/97#discussion_r429932827
##########
File path: tubemq-core/src/main/java/org/apache/tubemq/corebase/utils/AddressUtils.java
##########
@@ -34,31 +40,29 @@ public static synchronized String getLocalAddress() throws Exception {
return localIPAddress;
}
- public static boolean validLocalIp(String currLocalHost) throws Exception {
+ public static boolean validLocalIp(String currLocalHost) {
if (TStringUtils.isNotEmpty(localIPAddress)
&& localIPAddress.equals(currLocalHost)) {
return true;
}
- Enumeration<NetworkInterface> allInterface = NetworkInterface.getNetworkInterfaces();
- if (allInterface == null) {
- throw new Exception("Get NetworkInterfaces is null");
- }
+
+ Enumeration<NetworkInterface> allInterface = listNetworkInterface();
+
return checkValidIp(allInterface, currLocalHost);
}
- private static boolean checkValidIp(Enumeration<NetworkInterface> allInterface,
- String currLocalHost) throws Exception {
- String localIp = null;
+ private static boolean checkValidIp(Enumeration<NetworkInterface> allInterface, String currLocalHost) {
Review comment:
The initial idea is only for IPV4, if the IPV4 address cannot be obtained on the machine. Will cause an exception to the user, let the user specify the IP address, which is why the two constructors are not marked with @deprecated annotation.
Now, I understand that IPV4 and IPV6 are used together. In that case, I will reorganize the process
There are two options:
First, get the address completely automatically
1. Priority access to IPV4 address.
2. If not available, obtain an IPV6 address.
3. No IP can be obtained, choose one of the two below
1) Throw the exception to the user
2) Get IP from InetAddress
Second, give priority to obtain IP from the designated network interface
1. Try to obtain IPV4 from the specified network card. If not, obtain IPV4 from other network interfaces.
2. If IPV4 is not obtained in the previous step, try to obtain IPV6 from the specified network interface. Otherwise, obtain IPV6 from other network interfaces.
3. No IP can be obtained, choose one of the two below
1) Throw the exception to the user
2) Get IP from InetAddress
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tubemq] gosonzhang commented on a change in pull request #97: [TUBEMQ-141] Remove the requirement to provide localHostIP
Posted by GitBox <gi...@apache.org>.
gosonzhang commented on a change in pull request #97:
URL: https://github.com/apache/incubator-tubemq/pull/97#discussion_r431196936
##########
File path: tubemq-core/src/main/java/org/apache/tubemq/corebase/utils/AddressUtils.java
##########
@@ -130,4 +137,50 @@ public static String getRemoteAddressIP(Channel channel) {
}
return strRemoteIP;
}
+
+ public static Enumeration<NetworkInterface> listNetworkInterface() {
+ try {
+ return NetworkInterface.getNetworkInterfaces();
+ } catch (SocketException e) {
+ throw new AddressException(e);
+ }
+ }
+
+ public static String getIPV4LocalAddress() {
+ try {
+ return doGetIPV4LocalAddress();
+ } catch (SocketException | UnknownHostException e) {
+ String errorMsg = "Unable to obtain local IP, please check if the local network is normal";
+ throw new AddressException(errorMsg, e);
+ }
+ }
+
+ private static String doGetIPV4LocalAddress() throws SocketException, UnknownHostException {
+ Enumeration<NetworkInterface> enumeration = NetworkInterface.getNetworkInterfaces();
+
+ while (enumeration.hasMoreElements()) {
+ NetworkInterface networkInterface = enumeration.nextElement();
+ if ("docker0".equals(networkInterface.getName()) || !networkInterface.isUp()) {
Review comment:
Need to add NetworkInterface isLoopback() status judgment
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tubemq] gosonzhang commented on a change in pull request #97: [TUBEMQ-141] Remove the requirement to provide localHostIP
Posted by GitBox <gi...@apache.org>.
gosonzhang commented on a change in pull request #97:
URL: https://github.com/apache/incubator-tubemq/pull/97#discussion_r429907954
##########
File path: tubemq-core/src/main/java/org/apache/tubemq/corebase/utils/AddressUtils.java
##########
@@ -34,31 +40,29 @@ public static synchronized String getLocalAddress() throws Exception {
return localIPAddress;
}
- public static boolean validLocalIp(String currLocalHost) throws Exception {
+ public static boolean validLocalIp(String currLocalHost) {
if (TStringUtils.isNotEmpty(localIPAddress)
&& localIPAddress.equals(currLocalHost)) {
return true;
}
- Enumeration<NetworkInterface> allInterface = NetworkInterface.getNetworkInterfaces();
- if (allInterface == null) {
- throw new Exception("Get NetworkInterfaces is null");
- }
+
+ Enumeration<NetworkInterface> allInterface = listNetworkInterface();
+
return checkValidIp(allInterface, currLocalHost);
}
- private static boolean checkValidIp(Enumeration<NetworkInterface> allInterface,
- String currLocalHost) throws Exception {
- String localIp = null;
+ private static boolean checkValidIp(Enumeration<NetworkInterface> allInterface, String currLocalHost) {
Review comment:
It is possible that the user prefers to obtain IP addresses from a certain network card first, and then the IPV4 type address is the best choice, so can the function achieve such an effect:
1. the program first obtains the IP address from the specified network card, and prefers the IPV4 address;
2. the specified network card does not Get IP from other network card when IPV4 address;
3. if there are no IPV4 address, I choose IPV6;
4. when all network cards can not get IP address, then get from InetAddress object?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tubemq] gosonzhang commented on a change in pull request #97: [TUBEMQ-141] Remove the requirement to provide localHostIP
Posted by GitBox <gi...@apache.org>.
gosonzhang commented on a change in pull request #97:
URL: https://github.com/apache/incubator-tubemq/pull/97#discussion_r431195383
##########
File path: tubemq-core/src/main/java/org/apache/tubemq/corebase/utils/AddressUtils.java
##########
@@ -34,31 +40,29 @@ public static synchronized String getLocalAddress() throws Exception {
return localIPAddress;
}
- public static boolean validLocalIp(String currLocalHost) throws Exception {
+ public static boolean validLocalIp(String currLocalHost) {
if (TStringUtils.isNotEmpty(localIPAddress)
&& localIPAddress.equals(currLocalHost)) {
return true;
}
- Enumeration<NetworkInterface> allInterface = NetworkInterface.getNetworkInterfaces();
- if (allInterface == null) {
- throw new Exception("Get NetworkInterfaces is null");
- }
+
+ Enumeration<NetworkInterface> allInterface = listNetworkInterface();
+
return checkValidIp(allInterface, currLocalHost);
}
- private static boolean checkValidIp(Enumeration<NetworkInterface> allInterface,
- String currLocalHost) throws Exception {
- String localIp = null;
+ private static boolean checkValidIp(Enumeration<NetworkInterface> allInterface, String currLocalHost) {
Review comment:
ok！
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tubemq] gosonzhang commented on a change in pull request #97: [TUBEMQ-141] Remove the requirement to provide localHostIP
Posted by GitBox <gi...@apache.org>.
gosonzhang commented on a change in pull request #97:
URL: https://github.com/apache/incubator-tubemq/pull/97#discussion_r429903694
##########
File path: tubemq-core/src/main/java/org/apache/tubemq/corebase/utils/AddressUtils.java
##########
@@ -130,4 +134,100 @@ public static String getRemoteAddressIP(Channel channel) {
}
return strRemoteIP;
}
+
Review comment:
What if there is only IPV6 on the machine?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tubemq] gosonzhang commented on a change in pull request #97: [TUBEMQ-141] Remove the requirement to provide localHostIP
Posted by GitBox <gi...@apache.org>.
gosonzhang commented on a change in pull request #97:
URL: https://github.com/apache/incubator-tubemq/pull/97#discussion_r429904199
##########
File path: tubemq-core/src/main/java/org/apache/tubemq/corebase/utils/AddressUtils.java
##########
@@ -130,4 +134,100 @@ public static String getRemoteAddressIP(Channel channel) {
}
return strRemoteIP;
}
+
+ public static String getLocalIPv4Address() {
+ List<String> ips = listNonLoopbackIPV4Address();
+
+ if (ips.isEmpty()) {
+ try {
+ return InetAddress.getLocalHost().getHostAddress();
+ } catch (UnknownHostException e) {
+ String errorMsg = "Unable to obtain local IP, please manually specify the local IP address";
+ throw new AddressException(errorMsg, e.getCause());
+ }
+ }
+
+ for (String ip : ips) {
+ if (ip.startsWith("192.168")) {
Review comment:
Why exclude "192.168" separately?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org