Skip to content

SPARK-4628: Put all external projects behind a build flag #3485

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

Closed
wants to merge 1 commit into from

Conversation

pwendell
Copy link
Contributor

This is important since we don't want breaks in the dependency
graphs of external projects to break the default Spark build.

@pwendell
Copy link
Contributor Author

Jenkins, test this please.

@@ -1201,6 +1196,18 @@
</dependencies>
</profile>

<!-- External projects are not built in less this flag is enabled. -->
<profile>
<id>external</id>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be external-projects?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes - yes it should

@SparkQA
Copy link

SparkQA commented Nov 26, 2014

Test build #23903 has started for PR 3485 at commit a1f575f.

  • This patch merges cleanly.

This is important since we don't want breaks in the dependency
graphs of external projects to break the default Spark build.
@SparkQA
Copy link

SparkQA commented Nov 26, 2014

Test build #23905 has started for PR 3485 at commit f3a3f13.

  • This patch merges cleanly.

@@ -1201,6 +1196,18 @@
</dependencies>
</profile>

<!-- External projects are not built in less this flag is enabled. -->
Copy link
Contributor

Choose a reason for hiding this comment

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

in less -> unless

@sryza
Copy link
Contributor

sryza commented Nov 26, 2014

external/kafka will still end up being built without the flag, right?

Also, will this not make the default build fail, because the examples depend on the external projects, and examples still get built automatically?

@SparkQA
Copy link

SparkQA commented Nov 26, 2014

Test build #23903 has finished for PR 3485 at commit a1f575f.

  • This patch fails to build.
  • This patch merges cleanly.
  • This patch adds no public classes.

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/23903/
Test FAILed.

@SparkQA
Copy link

SparkQA commented Nov 26, 2014

Test build #23905 has finished for PR 3485 at commit f3a3f13.

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

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/23905/
Test PASSed.

@pwendell
Copy link
Contributor Author

@sryza yeah I think you're right. Maybe we should as a more surgical fix just put the mqtt project behind a flag and for 1.3 we can clean it up properly (and maybe you could write this patch if you're interested).

@tdas
Copy link
Contributor

tdas commented Nov 26, 2014

@pwendell @sryza I agree with that as well. Lets just put mqtt behind the flag for now.

@tdas
Copy link
Contributor

tdas commented Nov 26, 2014

On second thought, does the current build of Spark break right now? If it does not, it may be fine to release Spark 1.2 as is.

@sryza
Copy link
Contributor

sryza commented Nov 27, 2014

I missed the context for why we would just put mqtt behind the flag. Something in its dependency graph is breaking the build?

@tdas
Copy link
Contributor

tdas commented Nov 27, 2014

@sryza I am trying to figure out the full extent of it on the current default build as well. The issue is the mqtt-client version 0.4.0 has been removed from certain repositories, which is causing breaks in the certain downstream systems. In case of Spark, I think it may cause builds of our previous versions to also break (havent tested myself). So this patch I think tries to make it more secure for the future, so that such build breaks in external systems do no break the building of core Spark.

@pwendell Please correct me if my understanding is incorrect.

@tdas
Copy link
Contributor

tdas commented Nov 27, 2014

I have made an alternative approach to upgrade MQTT from 0.4.0 to 1.0.1. See #3487 . I recommend going this route, rather than changing builds and stuff this late in the game.

@pwendell
Copy link
Contributor Author

@sryza the issue is that mqtt-client is not hosted in maven central, it's maintained in an external repository owned by the eclipse project and also (for some versions) in a repository maintained by the Spring project. This is really a bad situation because we are relying on an artifact could simply be removed at some point in the future without warning and would break any Spark build.

For instance, all Spark 0.9.X builds are now broken because the eclipse repository removed mqtt-client 0.4.0. If in the future the spring repository also removes this (the spring repo was added to our build in Spark 1.0) many versions of Spark will fail to build.

As long as this is in the default build configuration we expose ourself to this, so I'm proposing removing mqtt from the default build. We should have a firm policy moving forward that we can't depend on artifacts that aren't hosted in maven central.

@srowen
Copy link
Member

srowen commented Nov 27, 2014

I think updating to MQTT 1.0.1 per @tdas is a good move. However @prabeesh seems to have found the new home of this old artifact (haven't tested it): #3492 Maybe that is a good PR to back-port, while moving ahead with MQTT 1.0.1 in the short term.

@srowen
Copy link
Member

srowen commented Nov 27, 2014

Oops, I think I spoke too soon. I actually just clicked through the paho-releases repo and ...

https://repo.eclipse.org/content/repositories/paho-releases/org/eclipse/paho/mqtt-client/0.4.0/

... is an empty directory.

@prabeesh
Copy link
Contributor

@srowen they updated the repo

@pwendell
Copy link
Contributor Author

That's great that they updated the repo. Nonetheless I think we should put this module behind a build flag for the reasons stated earlier. We can't be dependent on a third party repo for our default build to work, in general. So for 1.2 I'll just have an mqtt flag. For 1.3 we separately had thought of isolating these from the default build more generally, but we can defer that. So I'll close this patch and open up a simpler one.

@pwendell pwendell closed this Nov 28, 2014
@pwendell
Copy link
Contributor Author

Actually even flagging only MQTT is complicated because of the examples. So I'll just punt on this until later. It just leaves us in a situation where we might have to make a patch release down the road if their repo changes, which isn't great.

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.

8 participants