Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SPARK-7205] Support .ivy2/local and .m2/repositories/ in --packages #5755

Closed
wants to merge 1 commit into from

Conversation

brkyvz
Copy link
Contributor

@brkyvz brkyvz commented Apr 28, 2015

In addition, I made a small change that will allow users to import 2 different artifacts with the same name. That change is made in [organization]_[artifact]-[revision].[ext]. This used to be only [artifact].[ext] which might have caused collisions between artifacts with the same artifactId, but different groupId's.

cc @pwendell

@SparkQA
Copy link

SparkQA commented Apr 28, 2015

Test build #31167 has finished for PR 5755 at commit c47c9c5.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.
  • This patch does not change any dependencies.

// We need a chain resolver if we want to check multiple repositories
val cr = new ChainResolver
cr.setName("list")

val localM2 = new IBiblioResolver
localM2.setM2compatible(true)
val m2Path = ".m2" + File.separator + "repository" + File.separator
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure that you can point ivy to a maven repository like this? Ivy and maven use different format for laying out directory structures (does setUsepoms just make this work?). I'd make sure you explicitly test this locally with the .m2 folder.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah I guess that comes with setM2compatible.

@pwendell
Copy link
Contributor

LGTM - but just to be sure, since the unit tests don't actually cover loading a jar from the local maven or ivy cache, have you tested it locally? If you wanted to go above and beyond, you could create unit tests that generate a maven or ivy dependency in temporary folder and then test that you can load it.

@brkyvz
Copy link
Contributor Author

brkyvz commented Apr 29, 2015

I tested it locally. I can add unit tests. In fact I have a Utils file that creates mock repositories wherever I want them. I can add that suite in a follow up PR (or on this one). It's about 180 lines. With that, we can get rid of the internet dependency (and potential flakiness).

@pwendell
Copy link
Contributor

Okay thanks - we have SPARK-7224 for dealing with better automated testing. I'll merge this now.

@asfgit asfgit closed this in f98773a Apr 29, 2015
jeanlyn pushed a commit to jeanlyn/spark that referenced this pull request May 28, 2015
In addition, I made a small change that will allow users to import 2 different artifacts with the same name. That change is made in `[organization]_[artifact]-[revision].[ext]`. This used to be only `[artifact].[ext]` which might have caused collisions between artifacts with the same artifactId, but different groupId's.

cc pwendell

Author: Burak Yavuz <brkyvz@gmail.com>

Closes apache#5755 from brkyvz/local-caches and squashes the following commits:

c47c9c5 [Burak Yavuz] Small fixes to --packages
andrewor14 pushed a commit to andrewor14/spark that referenced this pull request Jun 4, 2015
In addition, I made a small change that will allow users to import 2 different artifacts with the same name. That change is made in `[organization]_[artifact]-[revision].[ext]`. This used to be only `[artifact].[ext]` which might have caused collisions between artifacts with the same artifactId, but different groupId's.

cc pwendell

Author: Burak Yavuz <brkyvz@gmail.com>

Closes apache#5755 from brkyvz/local-caches and squashes the following commits:

c47c9c5 [Burak Yavuz] Small fixes to --packages

Conflicts:
	core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala
jeanlyn pushed a commit to jeanlyn/spark that referenced this pull request Jun 12, 2015
In addition, I made a small change that will allow users to import 2 different artifacts with the same name. That change is made in `[organization]_[artifact]-[revision].[ext]`. This used to be only `[artifact].[ext]` which might have caused collisions between artifacts with the same artifactId, but different groupId's.

cc pwendell

Author: Burak Yavuz <brkyvz@gmail.com>

Closes apache#5755 from brkyvz/local-caches and squashes the following commits:

c47c9c5 [Burak Yavuz] Small fixes to --packages
nemccarthy pushed a commit to nemccarthy/spark that referenced this pull request Jun 19, 2015
In addition, I made a small change that will allow users to import 2 different artifacts with the same name. That change is made in `[organization]_[artifact]-[revision].[ext]`. This used to be only `[artifact].[ext]` which might have caused collisions between artifacts with the same artifactId, but different groupId's.

cc pwendell

Author: Burak Yavuz <brkyvz@gmail.com>

Closes apache#5755 from brkyvz/local-caches and squashes the following commits:

c47c9c5 [Burak Yavuz] Small fixes to --packages
@@ -899,7 +916,8 @@ private[deploy] object SparkSubmitUtils {
}
// retrieve all resolved dependencies
ivy.retrieve(rr.getModuleDescriptor.getModuleRevisionId,
packagesDirectory.getAbsolutePath + File.separator + "[artifact](-[classifier]).[ext]",
packagesDirectory.getAbsolutePath + File.separator +
"[organization]_[artifact]-[revision].[ext]",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason we got rid of (-[classifier])?

I saw some discussions about this. For example,

If this pattern for instance doesn't has the [type] or [classifier] token, Ivy will download the source/javadoc artifacts to the same file as the regular jar.

asfgit pushed a commit that referenced this pull request Dec 20, 2017
## What changes were proposed in this pull request?
In the previous PR #5755 (comment), we dropped `(-[classifier])` from the retrieval pattern. We should add it back; otherwise,
> If this pattern for instance doesn't has the [type] or [classifier] token, Ivy will download the source/javadoc artifacts to the same file as the regular jar.

## How was this patch tested?
The existing tests

Author: gatorsmile <gatorsmile@gmail.com>

Closes #20037 from gatorsmile/addClassifier.
@brkyvz brkyvz deleted the local-caches branch February 3, 2019 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants