You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by advancedxy <gi...@git.apache.org> on 2014/08/14 17:53:57 UTC
[GitHub] spark pull request: [SPARK-3040] pick up a more proper local ip ad...
GitHub user advancedxy opened a pull request:
https://github.com/apache/spark/pull/1946
[SPARK-3040] pick up a more proper local ip address for Utils.findLocalIpAddress method
Short version: NetworkInterface.getNetworkInterfaces returns ifs in reverse order compared to ifconfig output.
See [SPARK_3040](https://issues.apache.org/jira/browse/SPARK-3040) for more detail
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/advancedxy/spark SPARK-3040
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/1946.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1946
----
commit 087a785ea9740ce2bb42c2d760b98c196678e5ad
Author: Ye Xianjin <ad...@gmail.com>
Date: 2014-08-14T15:45:46Z
reverse the Networkinterface.getNetworkInterfaces output order to
get a more proper local ip address.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3040] pick up a more proper local ip ad...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1946#issuecomment-52202890
Can one of the admins verify this patch?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3040] pick up a more proper local ip ad...
Posted by pwendell <gi...@git.apache.org>.
Github user pwendell commented on the pull request:
https://github.com/apache/spark/pull/1946#issuecomment-52384952
By risky I mean this - many existing users of Spark might depend on the current behavior, so if we suddenly chose a different interface it will break existing behavior and it could be non-obvious to users why this is happening.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3040] pick up a more proper local ip ad...
Posted by mridulm <gi...@git.apache.org>.
Github user mridulm commented on the pull request:
https://github.com/apache/spark/pull/1946#issuecomment-52395698
Thanks for digging into this !
From what I see, this is an implementation detail of a specific jdk version
(order in which syscall returns and order in which java handles it) and
subject to arbitrary change : I am not sure why the order is the way it is
as mentioned by you, but there might be some rationale for it .... would be
good to raise a jdk bug and get it clarified.
Which is not to say we can't include it in some future release of spark ,
if it makes end-user life easier - in case it is a jre limitations : as
Patrick mentioned, we need to ensure it does not break for existing users.
On 16-Aug-2014 9:41 am, "叶先进" <no...@github.com> wrote:
> @pwendell <https://github.com/pwendell> @mridulm
> <https://github.com/mridulm> no, this behavior is not documented. The
> java SE doc doesn't say anything about ordering.. So before filing the
> JIRA, I looked up the OpenJDK source code, both 6 and 7.
> Maybe I should post some snippets here.
> In openjdk-6-src-b27,
> jdk/src/windows/native/java/net/NetworkInterface.c Line335-
>
> /*
> * Put the interface at tail of list as GetIfTable(,,TRUE) is
> * returning the interfaces in index order.
> */
> count++;
> if (netifP == NULL) {
> netifP = curr;
> } else {
> netif *tail = netifP;
> while (tail->next != NULL) {
> tail = tail->next;
> }
> tail->next = curr;
> }
>
>
> jdk/src/solaris/native/java/net/NetworkInterface.c function *addif Line1000
>
> /*
> * If "new" then create an netif structure and
> * insert it onto the list.
> */
> if (currif == NULL) {
> currif = (netif *)malloc(sizeof(netif));
> if (currif) {
> currif->name = strdup(name);
> if (currif->name == NULL) {
> free(currif);
> currif = NULL;
> }
> }
> if (currif == NULL) {
> JNU_ThrowOutOfMemoryError(env, "heap allocation failed");
> return ifs;
> }
> currif->index = index;
> currif->addr = NULL;
> currif->childs = NULL;
> currif->virtual = isVirtual;
> currif->next = ifs;
> ifs = currif;
> }
>
> Of course, the whole flow is more complicated, but you can get the idea.
> Source code in OpenJDk7 is similar, I won't post it here as this is a long
> comment.
>
> —
> Reply to this email directly or view it on GitHub
> <https://github.com/apache/spark/pull/1946#issuecomment-52382756>.
>
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3040] pick up a more proper local ip ad...
Posted by advancedxy <gi...@git.apache.org>.
Github user advancedxy commented on the pull request:
https://github.com/apache/spark/pull/1946#issuecomment-52383319
And @pwendell, I don't think this is risky. It's a minor change and at least it don't do worse.
The current implementation doesn't pick the more proper ip on my laptop(mac os x) or a Debian.
Of course, if you don't want to put this into 1.1, I totally get it. But I hope this can be made into master and later release.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3040] pick up a more proper local ip ad...
Posted by advancedxy <gi...@git.apache.org>.
Github user advancedxy commented on the pull request:
https://github.com/apache/spark/pull/1946#issuecomment-52280562
Sorry, I didn't realize windows is supported. In that case, I believe a check is necessary. I will update the pr.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3040] pick up a more proper local ip ad...
Posted by pwendell <gi...@git.apache.org>.
Github user pwendell commented on the pull request:
https://github.com/apache/spark/pull/1946#issuecomment-55695674
Alright - let's see how this goes and we can rollback if we see any issues.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3040] pick up a more proper local ip ad...
Posted by mridulm <gi...@git.apache.org>.
Github user mridulm commented on the pull request:
https://github.com/apache/spark/pull/1946#issuecomment-52414914
Not sure, https://bugs.openjdk.java.net/browse/JDK ?
On Sun, Aug 17, 2014 at 8:54 AM, 叶先进 <no...@github.com> wrote:
> @mridulm <https://github.com/mridulm>, could you give me the link to
> report a jdk bug? It's very confusing that jdk has many different jira
> place (OpenJDK jira, Oracle etc.).
>
> After some second thought, maybe it's a bad idea to rely on the output
> order. Maybe we should iterate over all interfaces and get the best(or
> reasonable) ip.
>
> —
> Reply to this email directly or view it on GitHub
> <https://github.com/apache/spark/pull/1946#issuecomment-52412555>.
>
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3040] pick up a more proper local ip ad...
Posted by pwendell <gi...@git.apache.org>.
Github user pwendell commented on the pull request:
https://github.com/apache/spark/pull/1946#issuecomment-52348403
I do agree with @mridulm it's a somewhat risky change so I think it's out of scope to put it in 1.1. I think that it's a native method with no stated guarantees about ordering.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3040] pick up a more proper local ip ad...
Posted by pwendell <gi...@git.apache.org>.
Github user pwendell commented on the pull request:
https://github.com/apache/spark/pull/1946#issuecomment-52275310
I have seen cases where a bad interface was chose, so this does seem like a good idea. But for windows, does this mean that the wrong interface is chosen? Since we support windows, should we add a check here to confirm we are on a unix-like system?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3040] pick up a more proper local ip ad...
Posted by advancedxy <gi...@git.apache.org>.
Github user advancedxy commented on the pull request:
https://github.com/apache/spark/pull/1946#issuecomment-55064727
I'd love to see this get merged into 1.2.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3040] pick up a more proper local ip ad...
Posted by advancedxy <gi...@git.apache.org>.
Github user advancedxy commented on the pull request:
https://github.com/apache/spark/pull/1946#issuecomment-52283168
@pwendell, would you look at this? It's a fairly simple fix. I don't have windows for primary use, so it's not confirmed on windows. I hope someone who uses windows can confirm this behavior.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3040] pick up a more proper local ip ad...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the pull request:
https://github.com/apache/spark/pull/1946#issuecomment-55144780
I'm not for or against the fix, but just wanted to point out that event the order the OS returns the interfaces is not really reliable. On my machine, where I constantly switch between wired and wireless, and often have both on, the order of eth0 vs. wlan0 seems to depend on the order they got connected more than anything.
So sure, the change sounds fine if it helps with a common case (or at least to make windows and unix behave more alike?), but really I don't see how anyone can rely on any behavior here since there is really no expected behavior of the underlying api.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3040] pick up a more proper local ip ad...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/1946#issuecomment-54694471
Can one of the admins verify this patch?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3040] pick up a more proper local ip ad...
Posted by mridulm <gi...@git.apache.org>.
Github user mridulm commented on the pull request:
https://github.com/apache/spark/pull/1946#issuecomment-52292439
Is this relying on documented behaviour or observed evidence ?
If latter, it is iffy at best to include this.
On 15-Aug-2014 1:29 pm, "叶先进" <no...@github.com> wrote:
> @pwendell <https://github.com/pwendell>, would you look at this? It's a
> fairly simple fix. I don't have windows for primary use, so it's not
> confirmed on windows. I hope someone who uses windows can confirm this
> behavior.
>
> —
> Reply to this email directly or view it on GitHub
> <https://github.com/apache/spark/pull/1946#issuecomment-52283168>.
>
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3040] pick up a more proper local ip ad...
Posted by pwendell <gi...@git.apache.org>.
Github user pwendell commented on the pull request:
https://github.com/apache/spark/pull/1946#issuecomment-55550076
In that case I'd propose merging this tentatively and if it causes issues in the 1.2 dev/QA cycle we can revert it.
I dug around a bunch, it looks like there really isn't clear behavior here, even within Linux variants because the code used to do this had to change a lot for IPv6 support.There isn't an ordering specification anywhere I can find in the internal API's in BSD/POSIX sockets that relate to this.
So it could be that this patch helps some and hurts others. As such, I'd be inclined to try it out and rollback if we find anyone is hurt by this (if it's not a strict win then we should bias towards the old behavior).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3040] pick up a more proper local ip ad...
Posted by advancedxy <gi...@git.apache.org>.
Github user advancedxy commented on the pull request:
https://github.com/apache/spark/pull/1946#issuecomment-52382756
@pwendell @mridulm no, this behavior is not documented. The java SE doc doesn't say anything about ordering.. So before filing the JIRA, I looked up the OpenJDK source code, both 6 and 7.
Maybe I should post some snippets here.
In openjdk-6-src-b27,
jdk/src/windows/native/java/net/NetworkInterface.c Line335-
```
/*
* Put the interface at tail of list as GetIfTable(,,TRUE) is
* returning the interfaces in index order.
*/
count++;
if (netifP == NULL) {
netifP = curr;
} else {
netif *tail = netifP;
while (tail->next != NULL) {
tail = tail->next;
}
tail->next = curr;
}
```
jdk/src/solaris/native/java/net/NetworkInterface.c function *addif Line1000
```
/*
* If "new" then create an netif structure and
* insert it onto the list.
*/
if (currif == NULL) {
currif = (netif *)malloc(sizeof(netif));
if (currif) {
currif->name = strdup(name);
if (currif->name == NULL) {
free(currif);
currif = NULL;
}
}
if (currif == NULL) {
JNU_ThrowOutOfMemoryError(env, "heap allocation failed");
return ifs;
}
currif->index = index;
currif->addr = NULL;
currif->childs = NULL;
currif->virtual = isVirtual;
currif->next = ifs;
ifs = currif;
}
```
Of course, the whole flow is more complicated, but you can get the idea.
Source code in OpenJDk7 is similar, I won't post it here as this is a long comment.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3040] pick up a more proper local ip ad...
Posted by pwendell <gi...@git.apache.org>.
Github user pwendell commented on the pull request:
https://github.com/apache/spark/pull/1946#issuecomment-54876248
On this one I'm a little torn - mostly because I've directly experienced this issue and also I've seen users run into it, so I think it's a fairly severe issue in terms of how we chose the default. The current behavior is indeed backwards in the most common case (someone running oracle or openJDK on Linux/Unix).
For that reason, I'm inclined to merge this and see if anyone runs into any case where this somehow regresses behavior, and if so we can consider reverting it for 1.2. When we are considering things like this it's generally good to merge them early in a release cycle so it gets a lot of testing.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3040] pick up a more proper local ip ad...
Posted by advancedxy <gi...@git.apache.org>.
Github user advancedxy commented on the pull request:
https://github.com/apache/spark/pull/1946#issuecomment-52412555
@mridulm, could you give me the link to report a jdk bug? It's very confusing that jdk has many different jira place (OpenJDK jira, Oracle etc.).
After some second thought, maybe it's a bad idea to rely on the output order. Maybe we should iterate over all interfaces and get the best(or reasonable) ip.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3040] pick up a more proper local ip ad...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/1946
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org