Skip to content

Conversation

@zsxwing
Copy link
Member

@zsxwing zsxwing commented Aug 22, 2015

This PR removed the outputFile configuration from pom.xml and updated tests.py to search jars for both sbt build and maven build.

I ran mvn -Pkinesis-asl -DskipTests clean install locally, and verified the jars in my local repository were correct. I also checked Python tests for maven build, and it passed all tests.

@srowen
Copy link
Member

srowen commented Aug 22, 2015

CC @jerryshao since in https://github.com/apache/spark/pull/5632/files I think it was argued that this is on purpose. Also never hurts to CC @vanzin

@SparkQA
Copy link

SparkQA commented Aug 22, 2015

Test build #41408 has finished for PR 8373 at commit be1d8a5.

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

@SparkQA
Copy link

SparkQA commented Aug 22, 2015

Test build #41410 has finished for PR 8373 at commit c697627.

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

@SparkQA
Copy link

SparkQA commented Aug 22, 2015

Test build #41412 has finished for PR 8373 at commit e0b5818.

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

@jerryshao
Copy link
Contributor

The code LGTM.

A simple question: any specific reason to change the Maven output file path back to default? IMHO it would be better to keep this output file path to be the same as sbt, also be consistent with other modules like assembly and example.

@zsxwing
Copy link
Member Author

zsxwing commented Aug 23, 2015

@jerryshao see https://issues.apache.org/jira/browse/SPARK-10168 If we set the output path, there will be still a jar in the target folder and maven will publish it rather than the jar specified by the output path.

@srowen
Copy link
Member

srowen commented Aug 23, 2015

@zsxwing yes but the point is that it is now different from the SBT output path. Why is it required that these be different? It's possible, but I think it needs a motivation.

@zsxwing
Copy link
Member Author

zsxwing commented Aug 23, 2015

@zsxwing yes but the point is that it is now different from the SBT output path. Why is it required that these be different? It's possible, but I think it needs a motivation.

So you are suggesting to fix the sbt output path. Right?

@zsxwing
Copy link
Member Author

zsxwing commented Aug 23, 2015

I'm not sure if we can change the output path for SBT. By default, it will put all generated files in target/scala-***/ folder, since it supports to build artifacts for multiple Scala versions at the same time.

@srowen
Copy link
Member

srowen commented Aug 23, 2015

No, I'm wondering why the output path has to change?

@zsxwing
Copy link
Member Author

zsxwing commented Aug 23, 2015

E.g., in 1.5.0-rc1, for the kafka-assembly project, maven will generate a jar without any codes except some meta files in the target folder, see https://repository.apache.org/content/repositories/orgapachespark-1137/org/apache/spark/spark-streaming-kafka-assembly_2.10/1.5.0-rc1/ for example.

Because outputPath was set, maven-shaded-plugin didn't replace spark-streaming-kafka-assembly_2.10-1.5.0-rc1.jar with the new real assembly jar, it just put the assembly jar in the outputPath. So maven just published this empty jar. By default, maven publishes the file in the target folder, rather than the path specified by outputPath of maven-shaded-plugin.

That's why I removed outputPath. Then maven-shaded-plugin will replace spark-streaming-kafka-assembly_2.10-1.5.0-rc1.jar with the new real assembly jar in the same path (the default behavior of maven-shaded-plugin). So that maven will publish the correct assembly jar.

@tdas
Copy link
Contributor

tdas commented Aug 24, 2015

I think I understand the problem and the fix strategy sounds good to me. But I will leave it @srowen to LGTM this as he is more well-versed with pom stuff than I am. Also, @jerryshao please LGTM it if you think this change does not regression, that is, it does not recreate the bug that you tried to fix in #5632.

@srowen
Copy link
Member

srowen commented Aug 24, 2015

Ah I see. So the release plugin isn't going to know about the updated output path to the shade plugin? then I completely agree it has to change as it won't work otherwise. Obviously more important than consistency.

@vanzin
Copy link
Contributor

vanzin commented Aug 24, 2015

LGTM. Yeah, I've been bitten by this before - the maven install plugin does not understand outputFile.

@tdas
Copy link
Contributor

tdas commented Aug 24, 2015

Alright. I am going to merge this to master and branch 1.5. Thanks @srowen, @vanzin and @jerryshao for taking a look.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch that this was missing! Thanks @zsxwing

asfgit pushed a commit that referenced this pull request Aug 24, 2015
…tifact jars

This PR removed the `outputFile` configuration from pom.xml and updated `tests.py` to search jars for both sbt build and maven build.

I ran ` mvn -Pkinesis-asl -DskipTests clean install` locally, and verified the jars in my local repository were correct. I also checked Python tests for maven build, and it passed all tests.

Author: zsxwing <zsxwing@gmail.com>

Closes #8373 from zsxwing/SPARK-10168 and squashes the following commits:

e0b5818 [zsxwing] Fix the sbt build
c697627 [zsxwing] Add the jar pathes to the exception message
be1d8a5 [zsxwing] Fix the issue that maven publishes wrong artifact jars

(cherry picked from commit 4e0395d)
Signed-off-by: Tathagata Das <tathagata.das1565@gmail.com>
@asfgit asfgit closed this in 4e0395d Aug 24, 2015
@zsxwing zsxwing deleted the SPARK-10168 branch August 25, 2015 03:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants