[GitHub] spark pull request: [SPARK-12267][Core]Store the remote RpcEnv add...

[zsxwing]

Github user zsxwing commented on the pull request:

https://github.com/apache/spark/pull/10261#issuecomment-164229994
  
> Akka doesn't have this problem as far as I understand the code; also 
because the akka rpc backend never runs in "client mode" (that's a netty 
rpc-only thing).

I didn't mean that. I just want to mention that if we want to change the 
semantics of `senderAddress`, we need to update the Akka RPC as well.

> BTW I'm fine with this change for 1.6; we can revisit this later to avoid 
having to keep extra state in the RPC layer.

Great. I'm going to merge this one to master and 1.6. Let's revisit this 
later.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-12267][Core]Store the remote RpcEnv add...

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/spark/pull/10261


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-12267][Core]Store the remote RpcEnv add...

[zsxwing]

Github user zsxwing commented on the pull request:

https://github.com/apache/spark/pull/10261#issuecomment-164231271
  
@vanzin I created 
[SPARK-12308](https://issues.apache.org/jira/browse/SPARK-12308) to remind us 
of your approach


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-12267][Core]Store the remote RpcEnv add...

[vanzin]

Github user vanzin commented on the pull request:

https://github.com/apache/spark/pull/10261#issuecomment-164177066
  
Akka doesn't have this problem as far as I understand the code; also 
because the akka rpc backend never runs in "client mode" (that's a netty 
rpc-only thing).


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-12267][Core]Store the remote RpcEnv add...

[vanzin]

Github user vanzin commented on the pull request:

https://github.com/apache/spark/pull/10261#issuecomment-164177137
  
BTW I'm fine with this change for 1.6; we can revisit this later to avoid 
having to keep extra state in the RPC layer.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-12267][Core]Store the remote RpcEnv add...

[AmplabJenkins]

Github user AmplabJenkins commented on the pull request:

https://github.com/apache/spark/pull/10261#issuecomment-164059104
  
Test PASSed.
Refer to this link for build results (access rights to CI server needed): 
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/47588/
Test PASSed.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-12267][Core]Store the remote RpcEnv add...

[AmplabJenkins]

Github user AmplabJenkins commented on the pull request:

https://github.com/apache/spark/pull/10261#issuecomment-164059097
  
Merged build finished. Test PASSed.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-12267][Core]Store the remote RpcEnv add...

[SparkQA]

Github user SparkQA commented on the pull request:

https://github.com/apache/spark/pull/10261#issuecomment-164058531
  
**[Test build #47588 has 
finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/47588/consoleFull)**
 for PR 10261 at commit 
[`3b37070`](https://github.com/apache/spark/commit/3b370702fc0be2de7dcbee1fa8c1c3ac3cc60cb5).
 * This patch passes all tests.
 * This patch merges cleanly.
 * This patch adds no public classes.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-12267][Core]Store the remote RpcEnv add...

[zsxwing]

Github user zsxwing commented on the pull request:

https://github.com/apache/spark/pull/10261#issuecomment-164110300
  
I see. However, for 1.6, we still need to support Akka RPC. Is it possible 
to get the client address from `ActorRef`? 


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-12267][Core]Store the remote RpcEnv add...

[vanzin]

Github user vanzin commented on the pull request:

https://github.com/apache/spark/pull/10261#issuecomment-164107825
  
> I saw your following comment...But I don't get it.

That's because we took opposite approaches. Your PR's approach is "if this 
connection says the sender address is a listening socket, then also consider 
that address when sending events about the remote process".

My PR takes the opposite approach: the address of the remote process is 
always the address of the socket used to connect, regardless of whether its 
also listening in another socket.

I think my approach is in the end more correct, but requires more code to 
fix existing code. In my view, `RpcCallContext.senderAddress` should be the 
address of the socket that sent the message, not the address the remote process 
is listening on.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-12267][Core]Store the remote RpcEnv add...

[zsxwing]

Github user zsxwing commented on the pull request:

https://github.com/apache/spark/pull/10261#issuecomment-164037328
  
@vanzin 

I saw your following comment in 
https://github.com/vanzin/spark/commit/3848bf5e5bee4aa132aa001baab41dc58d39e5c5#diff-acd05d6d379b6ef6ccf36bd3db5614f6R69
```
Because the messages used to register workers and applications were one-way
messages, though, "receive" was used, and that information was not 
available.
So now those messages are send using "ask", which looks a bit awkward but is
simpler than changing the RpcEnv API so that the client address is available
in the "receive" method.
```
But I don't get it.

The communication in master-worker, worker-driver and  master-driver are 
all in non-client mode. So for one-way messages, `RequestMessage.senderAddress` 
is the RpcEnv listening address. If we maintain the address relation like this 
PR, RpcEndpoint doesn't need to worry about the address issue even for 
`one-way` messages. Right?


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-12267][Core]Store the remote RpcEnv add...

[SparkQA]

Github user SparkQA commented on the pull request:

https://github.com/apache/spark/pull/10261#issuecomment-164036553
  
**[Test build #47588 has 
started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/47588/consoleFull)**
 for PR 10261 at commit 
[`3b37070`](https://github.com/apache/spark/commit/3b370702fc0be2de7dcbee1fa8c1c3ac3cc60cb5).


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-12267][Core]Store the remote RpcEnv add...

[zsxwing]

Github user zsxwing commented on the pull request:

https://github.com/apache/spark/pull/10261#issuecomment-163817812
  
Cool. I will close this PR.

On Thu, Dec 10, 2015 at 6:28 PM Marcelo Vanzin 
wrote:

> So, I don't think this is gonna work, and that's why my change is taking
> longer than I hoped to finish... while this might fix the immediate 
problem
> listed in the bug, there are other problems related to this that need to 
be
> fixed.
>
> Basically, standalone master HA is currently broken, and because there are
> no unit tests at all for that part of the code, nobody caught it...
>
> I'm trying to fix it, I think I almost got it, have one last exception to
> get rid of.
>
> —
> Reply to this email directly or view it on GitHub
> .
>



---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-12267][Core]Store the remote RpcEnv add...

[zsxwing]

Github user zsxwing commented on a diff in the pull request:

https://github.com/apache/spark/pull/10261#discussion_r47314731
  
--- Diff: core/src/main/scala/org/apache/spark/rpc/netty/NettyRpcEnv.scala 
---
@@ -580,6 +583,12 @@ private[netty] class NettyRpcHandler(
   // Create a new message with the socket address of the client as the 
sender.
   RequestMessage(clientAddr, requestMessage.receiver, 
requestMessage.content)
 } else {
+  // The remote RpcEnv listens to some port, we should also fire a 
RemoteProcessConnected for
+  // the listening address
+  val remoteEnvAddress = requestMessage.senderAddress
+  if (remoteAddresses.putIfAbsent(clientAddr, remoteEnvAddress) == 
null) {
+dispatcher.postToAll(RemoteProcessConnected(remoteEnvAddress))
--- End diff --

We don't need to concern about multiple network messages with different 
addresses since the codes already handle the bad addresses.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-12267][Core]Store the remote RpcEnv add...

[SparkQA]

Github user SparkQA commented on the pull request:

https://github.com/apache/spark/pull/10261#issuecomment-163816221
  
**[Test build #47561 has 
started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/47561/consoleFull)**
 for PR 10261 at commit 
[`f84e325`](https://github.com/apache/spark/commit/f84e3254c56a8617765d68086cee1220f4a22dfa).


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-12267][Core]Store the remote RpcEnv add...

[vanzin]

Github user vanzin commented on the pull request:

https://github.com/apache/spark/pull/10261#issuecomment-163816892
  
So, I don't think this is gonna work, and that's why my change is taking 
longer than I hoped to finish... while this might fix the immediate problem 
listed in the bug, there are other problems related to this that need to be 
fixed.

Basically, standalone master HA is currently broken, and because there are 
no unit tests at all for that part of the code, nobody caught it...

I'm trying to fix it, I think I almost got it, have one last exception to 
get rid of.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-12267][Core]Store the remote RpcEnv add...

[zsxwing]

Github user zsxwing commented on the pull request:

https://github.com/apache/spark/pull/10261#issuecomment-163810210
  
CC @vanzin  could you take a look? Thanks!


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-12267][Core]Store the remote RpcEnv add...

[zsxwing]

GitHub user zsxwing opened a pull request:

https://github.com/apache/spark/pull/10261

[SPARK-12267][Core]Store the remote RpcEnv address to send the correct 
disconnetion message



You can merge this pull request into a Git repository by running:

$ git pull https://github.com/zsxwing/spark SPARK-12267

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/spark/pull/10261.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 #10261


commit f84e3254c56a8617765d68086cee1220f4a22dfa
Author: Shixiong Zhu 
Date:   2015-12-11T02:05:17Z

Store the remote RpcEnv address to send the correct disconnetion message




---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-12267][Core]Store the remote RpcEnv add...

[marmbrus]

Github user marmbrus commented on the pull request:

https://github.com/apache/spark/pull/10261#issuecomment-163819194
  
hey guys, I know this is still in flight, but we are pretty behind on the 
release.  I'm going to cut RC2 and we can always make a judgement call about 
doing an RC3 once this is fixed.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-12267][Core]Store the remote RpcEnv add...

[AmplabJenkins]

Github user AmplabJenkins commented on the pull request:

https://github.com/apache/spark/pull/10261#issuecomment-163855910
  
Merged build finished. Test FAILed.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-12267][Core]Store the remote RpcEnv add...

[SparkQA]

Github user SparkQA commented on the pull request:

https://github.com/apache/spark/pull/10261#issuecomment-163855821
  
**[Test build #47561 has 
finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/47561/consoleFull)**
 for PR 10261 at commit 
[`f84e325`](https://github.com/apache/spark/commit/f84e3254c56a8617765d68086cee1220f4a22dfa).
 * This patch **fails from timeout after a configured wait of \`250m\`**.
 * This patch merges cleanly.
 * This patch adds no public classes.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-12267][Core]Store the remote RpcEnv add...

[AmplabJenkins]

Github user AmplabJenkins commented on the pull request:

https://github.com/apache/spark/pull/10261#issuecomment-163855911
  
Test FAILed.
Refer to this link for build results (access rights to CI server needed): 
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/47561/
Test FAILed.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org