[SPARK-23785][LAUNCHER] LauncherBackend doesn't check state of connection before setting state by sahilTakiar · Pull Request #20893 · apache/spark

sahilTakiar · 2018-03-23T20:26:14Z

What changes were proposed in this pull request?

Changed LauncherBackendset method so that it checks if the connection is open or not before writing to it (uses isConnected).

How was this patch tested?

None

…tion before setting state

jiangxb1987 · 2018-03-26T04:27:49Z

+ _isConnected = false
 super.close()
 } finally {
 onDisconnected()


I searched the code and seems this is a no-op?

_isConnected is used in def isConnected(). Moving it from the finally block to before the call to super.close() avoids a race condition where a client tries to write to the connection after it has been closed.

I know your meaning, but I'm not referring to isConnected, I mean onDisconnected seems not doing anything on Spark side.

This is not quite directly related, but this should means it is safe to move _isConnected = false to before onDisconnected().

Yeah, your right, it doesn't look like onDisconnected is being used anywhere, but its marked as protected so I guess its meant to be used by a sub-class (although no sub-class uses it).
So agree, this change should be safe, it doesn't change the semantics of any use of onDisconnected

jiangxb1987 · 2018-03-26T04:28:26Z

The change looks good, cc @cloud-fan

felixcheung · 2018-03-26T04:36:14Z

Jenkins, ok to test

SparkQA · 2018-03-26T07:05:01Z

Test build #88577 has finished for PR 20893 at commit 4ca8a32.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

vanzin · 2018-03-26T21:41:09Z

retest this please

vanzin · 2018-03-26T21:44:10Z

The change looks ok but the test being added is totally unrelated to it.

If you want to write a test, it should be not too hard to add something to SparkLauncherSuite in core/. Start an in-process app that waits on a lock, disconnect the handle, then wake up the lock so the app finishes successfully. That should fail without your fix and work with it.

sahilTakiar · 2018-03-26T23:55:28Z

Ok, I'll work on writing a test for SparkLauncherSuite.

The test added here was meant to cover the race condition mentioned here

SparkQA · 2018-03-27T01:11:08Z

Test build #88604 has finished for PR 20893 at commit 4ca8a32.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

sahilTakiar · 2018-03-28T02:21:44Z

Wrote a test in SparkLauncherSuite and was able to replicate the error from HIVE-18533, and then realized the exception is only logged and then swallowed. From SparkContext

 if (_dagScheduler != null) {
Utils.tryLogNonFatalError {
_dagScheduler.stop()
}
_dagScheduler = null
}

Seems the failures I was seeing in HIVE-18533 are due to something else.

Regardless, this is still probably a good fix since you still don't want to write to the connection unless its open, but given that the exception is only logged and not thrown, don't see an easy way to write a test for this.

vanzin · 2018-03-28T22:12:37Z

SGTM. Jenkins, retest this please.

vanzin · 2018-03-28T22:15:47Z

retest this please

SparkQA · 2018-03-29T02:18:38Z

Test build #88679 has finished for PR 20893 at commit 4ca8a32.

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

vanzin · 2018-03-29T17:22:58Z

Merging to master / 2.3.

…tion before setting state ## What changes were proposed in this pull request? Changed `LauncherBackend` `set` method so that it checks if the connection is open or not before writing to it (uses `isConnected`). ## How was this patch tested? None Author: Sahil Takiar <stakiar@cloudera.com> Closes#20893 from sahilTakiar/master. (cherry picked from commit 491ec11) Signed-off-by: Marcelo Vanzin <vanzin@cloudera.com>

dongjoon-hyun · 2018-03-30T17:19:02Z

Hi, @vanzin and @sahilTakiar .

Could you take a look at branch-2.3 hadoop-2.6 parts? It dosen't happen for all branches, but it seems to become unstable a little bit.

branch-2.3

master

vanzin · 2018-03-30T17:24:46Z

Let me revert the change from 2.3 and open a bug for the flaky test.

dongjoon-hyun · 2018-03-30T17:38:49Z

Thank you so much for swift action, @vanzin !

gatorsmile · 2018-04-02T21:31:01Z

Could we revert the PR from the master brach too?

https://amplab.cs.berkeley.edu/jenkins/view/Spark%20QA%20Test/job/spark-master-test-sbt-hadoop-2.7/4303/testReport/org.apache.spark.launcher/LauncherServerSuite/testAppHandleDisconnect/history/

gatorsmile · 2018-04-02T21:31:26Z

cc @vanzin @sahilTakiar @jiangxb1987

gatorsmile · 2018-04-02T21:32:37Z

Let me revert this PR from the master. Please resubmit the PR after addressing the flaky tests. Thanks!

vanzin · 2018-04-02T21:32:58Z

The test is fixed by #20950. Just push that one instead.

gatorsmile · 2018-04-02T21:33:36Z

I see. Could you merge that PR?

vanzin · 2018-04-02T21:34:15Z

Sure. Although I'm not sure why other committers who review PRs don't just merge them too...

…tion before setting state ## What changes were proposed in this pull request? Changed `LauncherBackend` `set` method so that it checks if the connection is open or not before writing to it (uses `isConnected`). ## How was this patch tested? None Author: Sahil Takiar <stakiar@cloudera.com> Closesapache#20893 from sahilTakiar/master.

[SPARK-23785][LAUNCHER] LauncherBackend doesn't check state of connec…
4ca8a32
…tion before setting state

sahilTakiar force-pushed the master branch from 1896236 to 4ca8a32Compare March 24, 2018 02:10

jiangxb1987 reviewed Mar 26, 2018
View reviewed changes

asfgit closed this in 491ec11Mar 29, 2018

Uh oh!

Conversation

sahilTakiar commented Mar 23, 2018

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

jiangxb1987Mar 26, 2018

Choose a reason for hiding this comment

Uh oh!

sahilTakiarMar 26, 2018

Choose a reason for hiding this comment

Uh oh!

jiangxb1987Mar 27, 2018

Choose a reason for hiding this comment

Uh oh!

jiangxb1987Mar 27, 2018

Choose a reason for hiding this comment

Uh oh!

sahilTakiarMar 27, 2018

Choose a reason for hiding this comment

Uh oh!

jiangxb1987 commented Mar 26, 2018

Uh oh!

felixcheung commented Mar 26, 2018

Uh oh!

SparkQA commented Mar 26, 2018

Uh oh!

vanzin commented Mar 26, 2018

Uh oh!

vanzin commented Mar 26, 2018

Uh oh!

sahilTakiar commented Mar 26, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Mar 27, 2018

Uh oh!

sahilTakiar commented Mar 28, 2018

Uh oh!

vanzin commented Mar 28, 2018

Uh oh!

vanzin commented Mar 28, 2018

Uh oh!

SparkQA commented Mar 29, 2018

Uh oh!

vanzin commented Mar 29, 2018

Uh oh!

dongjoon-hyun commented Mar 30, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vanzin commented Mar 30, 2018

Uh oh!

dongjoon-hyun commented Mar 30, 2018

Uh oh!

gatorsmile commented Apr 2, 2018

Uh oh!

gatorsmile commented Apr 2, 2018

Uh oh!

gatorsmile commented Apr 2, 2018

Uh oh!

vanzin commented Apr 2, 2018

Uh oh!

gatorsmile commented Apr 2, 2018

Uh oh!

vanzin commented Apr 2, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

sahilTakiar commented Mar 26, 2018•
edited
Loading

dongjoon-hyun commented Mar 30, 2018•
edited
Loading