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

[zsxwing]

No description provided.

[zsxwing]

CC @vanzin could you take a look? Thanks!

[zsxwing]

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

[vanzin]

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.

[zsxwing]

Cool. I will close this PR.

On Thu, Dec 10, 2015 at 6:28 PM Marcelo Vanzin notifications@github.com
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
#10261 (comment).

[marmbrus]

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.

[SparkQA]

Test build #47561 has finished for PR 10261 at commit f84e325.

• This patch fails from timeout after a configured wait of 250m.
• This patch merges cleanly.
• This patch adds no public classes.

[zsxwing]

@vanzin

I saw your following comment in vanzin@3848bf5#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?

[SparkQA]

Test build #47588 has finished for PR 10261 at commit 3b37070.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[vanzin]

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.

[zsxwing]

I see. However, for 1.6, we still need to support Akka RPC. Is it possible to get the client address from ActorRef?

[vanzin]

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).

[vanzin]

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.

[zsxwing]

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.

[zsxwing]

@vanzin I created SPARK-12308 to remind us of your approach