[BEAM-362] Move aggregator scraping API onto Pipeline, make the support code private by kennknowles · Pull Request #699 · apache/beam

kennknowles · 2016-07-20T04:33:50Z

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.

This moves out aggregators from where they are into various places where they belong. Notes:

The runner utility for traversing a pipeline and gathering aggregators is moved out
to runners-core. It could reasonably live in the SDK if it had any other use. It is
always easier to re-introduce it that to remove it, so I have removed it for now.
The Dataflow runner has a new dependency on runners-core.
This is a private implementation detail dependency. But it cannot be shaded until
we move packages so everything is under org.apache.beam.runners.core.
The user-facing aggregator stuff is moved out of the sdk.runners namespace to
live alongside PipelineResult, of which is it a sub-part. We might consider all
of these living in a namespace having to do with interacting with a "job", but that
is future work. For now this is internally consistent. We have a thought/goal that
the sdk.runners namespace can go away.

kennknowles · 2016-07-20T04:39:40Z

R: @dhalperi note added dependency for Dataflow runner
R: @amitsela I added the os-maven-plugin to the Spark runner's pom. We have a transitive dependency that is somewhat badly behaved, for now. Once #539 goes in, it may no longer be necessary; I haven't looked closely. The transitive dependency may also be fixed by upstream upgrades by the time it becomes relevant.

amitsela · 2016-07-20T08:46:59Z

Though I'm not sure what's the issue with Travis. I'll take a look once I'm near a computer.

lukecwik · 2016-07-20T14:13:02Z

Ken, I commented on PR/681 that we can use a jar that contains all native library variants since only depending on one means that if the client that builds the application is running on an OS which is different than what the runner executes on it will fail to load the native library when executing. Once PR/681 is fixed up, I don't think you'll need to have the maven OS detection plugin.

kennknowles · 2016-07-20T16:43:59Z

@lukecwik yea, I know. Same answer as @dhalperi there. A bit unfortunate/temporary, but unblocks for now.

kennknowles · 2016-07-20T16:44:04Z

@amitsela Travis did pass prior to my opening the PR. Seems there was a Travis timeout/hang in the Mac build. "No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself."

I'll go ahead and rebase to kick Travis and see if it was an infrastructure issue.

kennknowles · 2016-07-20T17:00:37Z

Discussed offline a bit, with these conclusions:

To produce the JSON from the TransformHierarchy we are actually going to need this stuff in the SDK, but it should be properly private and have a nice API.
But we still do need to add the dependency from Dataflow runner to runners-core to eliminate duplication of e.g. UnboundedReadFromBoundedSource. But this dependency should be shaded immediately, not left as a future task.

I'll update this PR accordingly.

kennknowles · 2016-07-20T22:23:43Z

Based on comments here, I am going to cherry-pick the os-maven-plugin thing.

kennknowles · 2016-07-21T03:58:57Z

PTAL @dhalperi. It isn't perfect, but the support classes are now package-private. They were previously public and in a namespace we want to remove.

Could move the getAggregatorSteps back out, perhaps to some static utility class Pipelines or Aggregators, but it has to be public somewhere. The support class remains an implementation detail of that one method.

lukecwik · 2016-07-21T16:20:15Z

Ken, I just merged pr/701 so you should be able to drop the OS detection plugin now.

kennknowles · 2016-07-22T17:18:01Z

@lukecwikos-maven-plugin is no longer part of this PR.

kennknowles · 2016-07-22T17:18:30Z

Travis failure is the issue in the Mac infrastructure.

kennknowles · 2016-07-25T20:14:50Z

Rebased again - same result.

The Mac build hangs while fetching deps. Note that the pre-merge build of kennknowles/incubator-beam passed.

So either it is infrastructural, nondeterministic, or there's some Mac-facing bug in mainline. Or some combination. Note that other PRs eventually get past this by repeatedly retrying. I'd rather not bother with that, but don't want to ignore a flaky issue either. Either way, not related to this change.

kennknowles · 2016-07-28T03:20:01Z

R: -@amitsela (no longer relevant to Spark or particularly interesting in any way)

kennknowles · 2016-07-28T03:23:20Z

@dhalperi every failure for the last few rebases has been timeouts or hangs in Travis, various platforms, most recently all of them. There haven't been any code changes in a while. Jenkins has been happy all along. Do you have any comments about the code itself? I'll keep prodding the infrastructure to see if it passes or uncovers a real issue.

dhalperi · 2016-07-29T03:21:54Z

Sorry -- I've just been busy. Will TAL tomorrow.

kennknowles · 2016-07-29T04:33:47Z

It passed for a second :-)

But it had conflicts. I've rebased and pushed. Crossing fingers...

kennknowles · 2016-08-08T20:55:15Z

R: @tgroh maybe a first pass would be good

This class is trivial. Adding it to the public API of the SDK is not desirable, since it is just for runners. Adding it to runners-core would be OK but is really overkill for a glorified Map.

dhalperi · 2016-08-10T18:28:24Z

 }
 }

+ /**


s/between/from/ ?

dhalperi · 2016-08-10T18:29:02Z

LGTM, please self-merge.

use the pytest.mark.xfail decorator

kennknowles force-pushed the runners-core-aggregators branch from 3230461 to cfe0276Compare July 20, 2016 04:36

kennknowles force-pushed the runners-core-aggregators branch 2 times, most recently from fef52dd to dacf275Compare July 20, 2016 04:41

kennknowles force-pushed the runners-core-aggregators branch from dacf275 to 0869ab5Compare July 20, 2016 16:44

kennknowles force-pushed the runners-core-aggregators branch from 0869ab5 to a01444cCompare July 21, 2016 03:52

kennknowles changed the title ~~[BEAM-362] Move aggregator-scraping to runners-core~~[BEAM-362] Move aggregator scraping API onto Pipeline, make the support code privateJul 21, 2016

kennknowles force-pushed the runners-core-aggregators branch 2 times, most recently from ff464ea to 5700c5eCompare July 21, 2016 04:58

kennknowles force-pushed the runners-core-aggregators branch 2 times, most recently from 6aa9ebc to 8f67ae9Compare July 22, 2016 03:53

kennknowles force-pushed the runners-core-aggregators branch from 8f67ae9 to 52e5814Compare July 25, 2016 16:46

kennknowles force-pushed the runners-core-aggregators branch 2 times, most recently from 66c996c to 570aa11Compare July 28, 2016 03:15

kennknowles force-pushed the runners-core-aggregators branch from 570aa11 to f0b4b33Compare July 29, 2016 04:33

kennknowles force-pushed the runners-core-aggregators branch 4 times, most recently from 51d858c to 02e6bfdCompare August 4, 2016 21:44

kennknowles force-pushed the runners-core-aggregators branch 2 times, most recently from 52ea6d6 to 7f32d0eCompare August 5, 2016 21:17

kennknowles added 2 commits August 8, 2016 13:55

Fix MapAggregatorValues use of toStringHelper
c867790

Inline MapAggregatorValues to remove dependencies
9da4bbc
This class is trivial. Adding it to the public API of the SDK is not desirable, since it is just for runners. Adding it to runners-core would be OK but is really overkill for a glorified Map.

kennknowles force-pushed the runners-core-aggregators branch from 7f32d0e to e018b9eCompare August 8, 2016 20:55

dhalperi reviewed Aug 10, 2016
View reviewed changes

Comment threadsdks/java/core/src/main/java/org/apache/beam/sdk/Pipeline.java
}
}

/**
dhalperiAug 10, 2016
Copy link
Copy Markdown
Contributor
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/between/from/ ?

Move aggregator support classes out of runners namespace, make private
adec254

kennknowles force-pushed the runners-core-aggregators branch from e018b9e to adec254Compare August 10, 2016 18:34

asfgit merged commit adec254 into apache:masterAug 10, 2016

asfgit pushed a commit that referenced this pull request Aug 10, 2016
Thiscloses#699
fa0e395

kennknowles deleted the runners-core-aggregators branch November 12, 2016 03:01

pl04351820 pushed a commit to pl04351820/beam that referenced this pull request Dec 20, 2023
test: Mark document snapshot hash as flaky (apache#699)
9e4a018
use the pytest.mark.xfail decorator

  }
  }
+ /**

Uh oh!

Conversation

kennknowles commented Jul 20, 2016

Uh oh!

kennknowles commented Jul 20, 2016

Uh oh!

amitsela commented Jul 20, 2016

Uh oh!

lukecwik commented Jul 20, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kennknowles commented Jul 20, 2016

Uh oh!

kennknowles commented Jul 20, 2016

Uh oh!

kennknowles commented Jul 20, 2016

Uh oh!

kennknowles commented Jul 20, 2016

Uh oh!

kennknowles commented Jul 21, 2016

Uh oh!

lukecwik commented Jul 21, 2016

Uh oh!

kennknowles commented Jul 22, 2016

Uh oh!

kennknowles commented Jul 22, 2016

Uh oh!

kennknowles commented Jul 25, 2016

Uh oh!

kennknowles commented Jul 28, 2016

Uh oh!

kennknowles commented Jul 28, 2016

Uh oh!

dhalperi commented Jul 29, 2016

Uh oh!

kennknowles commented Jul 29, 2016

Uh oh!

kennknowles commented Aug 8, 2016

Uh oh!

dhalperiAug 10, 2016

Choose a reason for hiding this comment

Uh oh!

dhalperi commented Aug 10, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

lukecwik commented Jul 20, 2016•
edited
Loading