Skip to content

Conversation

@kennknowles
Copy link
Member

@kennknowles kennknowles commented Jul 8, 2016

Be sure to do all of the following to help us incorporate your contribution
quickly and easily:

  • Make sure the PR title is formatted like:
    [BEAM-<Jira issue #>] Description of pull request
  • Make sure tests pass via mvn clean verify. (Even better, enable
    Travis-CI on your fork and ensure the whole test matrix passes).
  • Replace <Jira issue #> in the title with the actual Jira issue
    number, if there is one.
  • If this contribution is large, please file an Apache
    Individual Contributor License Agreement.

Even an optional runtime dependency, such as from the examples
to a runner, does get pulled in for testing. This meant that
all the dependencies for all runners had to be resolvable in an
integration testing context. It is quite inconvenient.

Explicitly excluding runners via flags such as -pl !runners/spark
does not work since it causes errors in dependency resolution.

This change adds a profile, active by default, that can be
explicitly disabled to avoid pulling in dependencies.

Even an optional runtime dependency, such as from the examples
to a runner, does get pulled in for testing. This meant that
all the dependencies for all runners had to be resolvable in an
integration testing context. It is quite inconvient.

Explicitly excluding runners via flags such as `-pl !runners/spark`
does not work since it causes errors in dependency resolution.

This change adds a profile, active by default, that can be
explicitly disabled to avoid pulling in dependencies.
@kennknowles
Copy link
Member Author

R: @amitsela OR @aljoscha OR @dhalperi OR @jbonofre OR any committer, really.

@kennknowles
Copy link
Member Author

kennknowles commented Jul 8, 2016

I'd love to lift this up to the parent module, since it applies to both examples, but the way profiles are activated the Java 8 profile would turn it off and break the idea. But I hesitated to actually copy/paste it into the Java 7 examples module. And anyhow, the Java 7 examples have bigger issues, such as an actual compile-time dep on the Dataflow runner and also a cyclic dependency on the Spark runner until #539 is completed.

After #539, a trivial activation like JDK 1.4+ might be a way to have "active by default" that is not automatically deactivated.

@aljoscha
Copy link
Contributor

aljoscha commented Jul 8, 2016

Looks good to me!

@jbonofre
Copy link
Member

jbonofre commented Jul 8, 2016

LGTM

@dhalperi
Copy link
Contributor

dhalperi commented Jul 8, 2016

I think this needs a little more thought. Can we at least put each runner
in its own profile? Then hermetic builds can disable runners separately.
On Fri, Jul 8, 2016 at 01:27 Jean-Baptiste Onofré notifications@github.com
wrote:

LGTM


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#609 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAgIT2KMbWsMifILy9BV6IxGkkpznn9_ks5qTgnWgaJpZM4JHrkf
.

@kennknowles
Copy link
Member Author

I agree with the spirit, @dhalperi, but actually have a different perspective: The runners are not real dependencies of these runner-agnostic Beam examples, but a hack for convenient interactive use of this maven module by users who are exploring. It isn't really about enabling/disabling runners.

So it is the build without this new profile that is accurately configured. Hence there is a use case for surgically adding runners to the classpath (via the standard dependency mechanisms) but not for surgically removing them; the latter assumes a configuration that is somewhat erroneous by default.

It would actually be pretty ideal to have a profile activation that was specifically "interactive command line use" but I don't know if there is a clever way to make that happen.

@asfgit asfgit merged commit c17768f into apache:master Jul 8, 2016
asfgit pushed a commit that referenced this pull request Jul 8, 2016
@kennknowles kennknowles deleted the no-runners branch November 12, 2016 03:00
pl04351820 pushed a commit to pl04351820/beam that referenced this pull request Dec 20, 2023
Source-Link: googleapis/synthtool@d2871d9
Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-python:latest@sha256:b2dc5f80edcf5d4486c39068c9fa11f7f851d9568eea4dcba130f994ea9b5e97

Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
Amar3tto added a commit to akvelon/beam that referenced this pull request Apr 14, 2025
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.

5 participants