SPARK-10878 Fix race condition when multiple clients resolves artifacts at the same time by Victsm · Pull Request #18801 · apache/spark

Victsm · 2017-08-01T20:49:59Z

What changes were proposed in this pull request?

When multiple clients attempt to resolve artifacts via the "--packages" parameter, they could run into race condition when they each attempt to modify the dummy "org.apache.spark-spark-submit-parent-default.xml" file created in the default ivy cache dir.
This patch changes the behavior to encode timestamp in the dummy module descriptor so each client will operate on a different resolution file in the ivy cache dir. In addition, this patch changes the behavior of when and which resolution files are cleaned to prevent accumulation of resolution files in the default ivy cache dir.

How was this patch tested?

Unit test added in SparkSubmitUtilsSuite

…cts at the same time.

jiangxb1987 · 2017-08-06T15:24:09Z

Update the title:

[SPARK-10878][Core] Fix race condition when multiple clients resolves artifacts at the same time

jiangxb1987 · 2017-08-06T15:27:37Z

+ // Include timestamp in module name, so multiple clients resolving maven coordinates at the
+ // same time do not modify the same resolution file concurrently.
+ ModuleRevisionId.newInstance("org.apache.spark",
+ "spark-submit-parent-" + System.currentTimeMillis().toString,


nit: "spark-submit-parent-" + System.currentTimeMillis().toString -> s"spark-submit-parent-${System.currentTimeMillis}"

jiangxb1987 · 2017-08-06T15:40:43Z

 }
 }
+
+ test("test resolution files cleaned after resolving artifact") {


This test case doesn't test the senario we describe in the PR, that is, having multiple clients trying to resolve artifacts at the same time.

Actually triggering the original race in a test could be pretty hard, so I'm not sure that we necessarily should block this fix on having an end-to-end integration test which could reproduce the original race. I think that the use of the UUID should be sufficient and therefore the only important thing to test is that we're still cleaning up the files properly (as is being done here).
Therefore I would welcome a re-submitted and cleaned-up version of this PR which addresses the other review comments (which I hope to merge soon if it's in good shape).

jiangxb1987 · 2017-08-06T15:49:44Z

cc @JoshRosen @cloud-fan

kiszk · 2017-08-06T16:59:24Z

 /** A nice function to use in tests as well. Values are dummy strings. */
 def getModuleDescriptor: DefaultModuleDescriptor = DefaultModuleDescriptor.newDefaultInstance(
- ModuleRevisionId.newInstance("org.apache.spark", "spark-submit-parent", "1.0"))
+ // Include timestamp in module name, so multiple clients resolving maven coordinates at the


nit: coordinates -> coordinate ?

cloud-fan · 2017-08-07T06:48:59Z

ok to test

SparkQA · 2017-08-07T07:04:51Z

Test build #80324 has finished for PR 18801 at commit 1ace5cc.

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

cloud-fan · 2017-08-07T15:49:07Z

retest this please

SparkQA · 2017-08-07T18:45:20Z

Test build #80351 has finished for PR 18801 at commit 1ace5cc.

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

jiangxb1987 · 2017-08-08T10:55:21Z

Your test case don't reflect the change you made to support multiple clients resolves artifacts at the same time, could you add a new test case or manually test that? Thanks!

Victsm · 2017-08-08T23:31:08Z

@jiangxb1987 @kiszk
Thanks for the review.
Will update the PR shortly.

kiszk · 2017-09-08T18:08:37Z

@Victsm kindly ping

kiszk · 2017-10-04T00:52:44Z

gentle ping @Victsm

kiszk · 2017-11-06T07:06:53Z

gentle ping @Victsm

vanzin · 2017-12-12T00:20:04Z

+ // Include timestamp in module name, so multiple clients resolving maven coordinates at the
+ // same time do not modify the same resolution file concurrently.
+ ModuleRevisionId.newInstance("org.apache.spark",
+ "spark-submit-parent-" + System.currentTimeMillis().toString,


System.currentTimeMillis() is not very unique. If you want uniqueness, use a UUID, or better yet, java.nio.file.Files.createTempFile.
EDIT: this is not a file, so go with UUID.

vanzin · 2017-12-12T00:20:34Z

+ s"resolved-${mdId.getOrganisation}-${mdId.getName}-${mdId.getRevision}.properties")
+ )
+ currentResolutionFiles.foreach{ file =>
+ if (file.exists) file.delete


.exists() is not needed before .delete(). Also, add parentheses.

vanzin · 2017-12-12T00:20:50Z

+ new File(ivySettings.getDefaultCache,
+ s"resolved-${mdId.getOrganisation}-${mdId.getName}-${mdId.getRevision}.properties")
+ )
+ currentResolutionFiles.foreach{ file =>


space before {

squito · 2017-12-29T18:21:17Z

+1 on using UUID and having a better test case

kiszk · 2018-01-03T03:00:13Z

+1 for UUID

kiszk · 2018-01-03T03:00:43Z

gentle ping @Victsm

AmplabJenkins · 2018-03-22T04:57:40Z

Can one of the admins verify this patch?

gatorsmile · 2018-05-03T18:26:23Z

Anyone can take over this PR and submit it again?

kiszk · 2018-05-04T17:58:20Z

I can take over this PR.

gatorsmile · 2018-05-04T21:44:45Z

@kiszk Thank you!

… artifacts at the same time ## What changes were proposed in this pull request? When multiple clients attempt to resolve artifacts via the `--packages` parameter, they could run into race condition when they each attempt to modify the dummy `org.apache.spark-spark-submit-parent-default.xml` file created in the default ivy cache dir. This PR changes the behavior to encode UUID in the dummy module descriptor so each client will operate on a different resolution file in the ivy cache dir. In addition, this patch changes the behavior of when and which resolution files are cleaned to prevent accumulation of resolution files in the default ivy cache dir. Since this PR is a successor of #18801, close#18801. Many codes were ported from #18801. **Many efforts were put here. I think this PR should credit to Victsm .** ## How was this patch tested? added UT into `SparkSubmitUtilsSuite` Author: Kazuaki Ishizaki <ishizaki@jp.ibm.com> Closes#21251 from kiszk/SPARK-10878. (cherry picked from commit d3c426a) Signed-off-by: Marcelo Vanzin <vanzin@cloudera.com>

vanzin · 2018-05-10T21:44:34Z

@Victsm you can close this PR now.

… artifacts at the same time ## What changes were proposed in this pull request? When multiple clients attempt to resolve artifacts via the `--packages` parameter, they could run into race condition when they each attempt to modify the dummy `org.apache.spark-spark-submit-parent-default.xml` file created in the default ivy cache dir. This PR changes the behavior to encode UUID in the dummy module descriptor so each client will operate on a different resolution file in the ivy cache dir. In addition, this patch changes the behavior of when and which resolution files are cleaned to prevent accumulation of resolution files in the default ivy cache dir. Since this PR is a successor of apache#18801, closeapache#18801. Many codes were ported from apache#18801. **Many efforts were put here. I think this PR should credit to Victsm .** ## How was this patch tested? added UT into `SparkSubmitUtilsSuite` Author: Kazuaki Ishizaki <ishizaki@jp.ibm.com> Closesapache#21251 from kiszk/SPARK-10878.

… artifacts at the same time When multiple clients attempt to resolve artifacts via the `--packages` parameter, they could run into race condition when they each attempt to modify the dummy `org.apache.spark-spark-submit-parent-default.xml` file created in the default ivy cache dir. This PR changes the behavior to encode UUID in the dummy module descriptor so each client will operate on a different resolution file in the ivy cache dir. In addition, this patch changes the behavior of when and which resolution files are cleaned to prevent accumulation of resolution files in the default ivy cache dir. Since this PR is a successor of apache#18801, closeapache#18801. Many codes were ported from apache#18801. **Many efforts were put here. I think this PR should credit to Victsm .** added UT into `SparkSubmitUtilsSuite` Author: Kazuaki Ishizaki <ishizaki@jp.ibm.com> Closesapache#21251 from kiszk/SPARK-10878. (cherry picked from commit d3c426a) RB=1313943 G=superfriends-reviewers R=fli,mshen,yezhou,edlu A=fli

SPARK-10878 Fix race condidtion when multiple clients resolves artifa…
1ace5cc
…cts at the same time.

jiangxb1987 reviewed Aug 6, 2017
View reviewed changes

kiszk reviewed Aug 6, 2017
View reviewed changes

vanzin reviewed Dec 12, 2017
View reviewed changes

kiszk mentioned this pull request May 6, 2018
[SPARK-10878][core] Fix race condition when multiple clients resolves artifacts at the same time #21251
Closed

asfgit closed this in d3c426aMay 10, 2018

Uh oh!

Conversation

Victsm commented Aug 1, 2017

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

jiangxb1987 commented Aug 6, 2017

Uh oh!

jiangxb1987Aug 6, 2017

Choose a reason for hiding this comment

Uh oh!

jiangxb1987Aug 6, 2017

Choose a reason for hiding this comment

Uh oh!

JoshRosenMay 4, 2018

Choose a reason for hiding this comment

Uh oh!

jiangxb1987 commented Aug 6, 2017

Uh oh!

kiszkAug 6, 2017

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Aug 7, 2017

Uh oh!

SparkQA commented Aug 7, 2017

Uh oh!

cloud-fan commented Aug 7, 2017

Uh oh!

SparkQA commented Aug 7, 2017

Uh oh!

jiangxb1987 commented Aug 8, 2017

Uh oh!

Victsm commented Aug 8, 2017

Uh oh!

kiszk commented Sep 8, 2017

Uh oh!

kiszk commented Oct 4, 2017

Uh oh!

kiszk commented Nov 6, 2017

Uh oh!

vanzinDec 12, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vanzinDec 12, 2017

Choose a reason for hiding this comment

Uh oh!

vanzinDec 12, 2017

Choose a reason for hiding this comment

Uh oh!

squito commented Dec 29, 2017

Uh oh!

kiszk commented Jan 3, 2018

Uh oh!

kiszk commented Jan 3, 2018

Uh oh!

AmplabJenkins commented Mar 22, 2018

Uh oh!

gatorsmile commented May 3, 2018

Uh oh!

kiszk commented May 4, 2018

Uh oh!

gatorsmile commented May 4, 2018

Uh oh!

vanzin commented May 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

vanzinDec 12, 2017•
edited
Loading