-
Notifications
You must be signed in to change notification settings - Fork 26.4k
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
[Dubbo-2064] Ipv6 support #2079
Conversation
Why not create a IPV6Utils class? @ralf0131 :) |
Codecov Report
@@ Coverage Diff @@
## master #2079 +/- ##
============================================
+ Coverage 53.12% 54.59% +1.47%
- Complexity 4959 5127 +168
============================================
Files 559 559
Lines 24894 25195 +301
Branches 4432 4569 +137
============================================
+ Hits 13225 13756 +531
+ Misses 9650 9396 -254
- Partials 2019 2043 +24
Continue to review full report at Codecov.
|
@luyuanwan Good suggestion! For me, I think the current refactoring is not large enough to make that change. You can always improve it as you wish, and send the pull request. |
} catch (Throwable e) { | ||
logger.warn(e); | ||
} | ||
while (interfaces.hasMoreElements()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public static Enumeration<NetworkInterface> getNetworkInterfaces()
throws SocketException {
final NetworkInterface[] netifs = getAll();
// specified to return null if no network interfaces
if (netifs == null)
return null;
Should check the interfaces property null value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I am using Java 10, the implementation is different with Java 8. In Java 10 the implementation guarantees no null value is returned. I will add the null check.
} | ||
} catch (Throwable e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename e to ignored will be better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, this is just coding style. I prefer to use
catch (Throwable e) {
// ignore
}
instead of
catch(Throwable ignored) {
}
I found several codes in Dubbo's code that is written in the former way, but did not find my codes that is written in the way you suggested.
So I guess I am going to leave it unchanged.
} | ||
} catch (Throwable e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename e to ignored will be better
|
||
@Test | ||
public void testIsValidV6Address() { | ||
System.setProperty("java.net.preferIPv6Addresses", "true"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
String saved = System.setProperty("java.net.preferIPv6Addresses");
System.setProperty("java.net.preferIPv6Addresses", "true");
...
System.setProperty("java.net.preferIPv6Addresses", saved);
This way is more elegant and will not break subsequent tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
String saved = System.getProperty("java.net.preferIPv6Addresses");
System.setProperty("java.net.preferIPv6Addresses", "true");
...
System.setProperty("java.net.preferIPv6Addresses", saved);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, it is better.
@zonghaishang I've improved the code based on your review comments, could you take a look? |
It looks good to me. |
What is the purpose of the change
Add ipv6 support for Dubbo #2064
Brief changelog
java.net.preferIPv6Addresses
is set totrue
Verifying this change
How to test ipv6 support:
dubbo-demo-provider.xml
dubbo-demo-consumer.xml
Add
-Djava.net.preferIPv6Addresses=true
before starting Provider/Consumer.Follow this checklist to help us incorporate your contribution quickly and easily:
[Dubbo-XXX] Fix UnknownException when host config not exist #XXX
. Each commit in the pull request should have a meaningful subject line and body.mvn clean install -DskipTests
&mvn clean test-compile failsafe:integration-test
to make sure unit-test and integration-test pass.