Remove SparkPipelineOptionsFactory, SparkStreamingPipelineOptionsFactory

[tgroh]

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.

---

The SparkPipelineOptionsFactory should ensure that the returned options
have Spark as the PipelineRunner.

Use the SparkPipelineOptionsFactory in the spark TfIdf test

Without using the SparkPipelineRunner explicitly, the Pipeline run with
the SparkRunner may have an unexpected graph due to runner-specific
interceptions of the Pipeline#apply method

[tgroh]

R: @amitsela

[lukecwik]

@amitsela:
Why do we have SparkPipelineOptionsFactory?
Any reason not to just use PipelineOptionsFactory.as(SparkPipelineOptions.class)?

[amitsela]

@lukecwik I guess SparkPipelineOptionsFactory.create() was a wrapper for PipelineOptionsFactory.as(SparkPipelineOptions.class).

@tgroh It makes sense that if we provide a PipelineOptionsFactory.as wrapper to the Spark runner we should apply the SparkRunner there, but we should do the same in SparkStreamingPipelineOptionsFactory.create()

So it's either letting the user choose the correct options as @lukecwik suggests:
PipelineOptions options = PipelineOptionsFactory.as(SparkPipelineOptions.class) OR PipelineOptionsFactory.as(SparkStreamingPipelineOptions.class)
followed by options.setRunner(SparkPipelineRunner.class)

Or as @tgroh suggests:
SparkPipelineOptions opts = SparkPipelineOptionsFactory.create() OR SparkStreamingPipelineOptionsFactory.create()

As the model writers, what is the correct way for a runner implementor to choose ? Which way we want the user to use this ? should he explicitly state the Runner or choosing the options will provide the correct runner ?

[tgroh]

Generally Pipeline authors should not have to use any runner-specific classes during pipeline construction or submission; so users should call neither PipelineOptionsFactory.as(SparkPipelineOptions.class) nor SparkPipelineOptionsFactory.create(), and values for runner-specific options can be provided via PipelineOptionsFactory.fromArgs(String[]), with the actual runner implementation responsible for ensuring that the provided options are valid (via obtaining the appropriate subtype).

Pipeline authors are responsible for selecting a runner, currently at or before the time of Pipeline.create(), and at the time of Pipeline.run() as we move to the new runner API - they should do so, however, by using PipelineOptions.setRunner(Class<? extends PipelineRunner>) or passing the appropriate runner name in PipelineOptionsFactory.fromArgs(String[])

[lukecwik]

+1 for Thomas Groh response

On Tue, Apr 12, 2016 at 8:54 AM, Thomas Groh notifications@github.com
wrote:

Generally Pipeline authors should not have to use any runner-specific
classes during pipeline construction or submission; so users should call
neither PipelineOptionsFactory.as(SparkPipelineOptions.class) nor
SparkPipelineOptionsFactory.create(), and values for runner-specific
options can be provided via PipelineOptionsFactory.fromArgs(String[]),
with the actual runner implementation responsible for ensuring that the
provided options are valid (via obtaining the appropriate subtype).

Pipeline authors are responsible for selecting a runner, currently at or
before the time of Pipeline.create(), and at the time of Pipeline.run()
as we move to the new runner API - they should do so, however, by using PipelineOptions.setRunner(Class<?
extends PipelineRunner>) or passing the appropriate runner name in
PipelineOptionsFactory.fromArgs(String[])

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#167 (comment)

[amitsela]

I can see your point, and it's true for PipelineOptionsFactory.fromArgs(String[]).
I can still see why users who work mainly with one runner would benefit from a dedicated PipelineOptionsFactory, but in that case it should also set the runner (as done here by @tgroh )

so I guess it's +1 for me as well

[tgroh]

I've pushed a change to remove the factories, and fix up all of the tests in which they were used.

If this commit is merged, the first in this PR should be discarded.

[kennknowles]

@tgroh I don't quite understand your last comment. Perhaps you could just rebase to exactly what you propose to merge?

[tgroh]

@kennknowles done.

[amitsela]

+1 for removing the factories as well

[tgroh]

Rebased on top of beam. If there aren't any more comments, this is ready to merge, pending jenkins

[lukecwik]

LGTM