[BEAM-2076] Shade Runner Dependencies in the DirectRunner#2714
[BEAM-2076] Shade Runner Dependencies in the DirectRunner#2714tgroh wants to merge 2 commits intoapache:masterfrom
Conversation
runners/direct-java/pom.xml
Outdated
| <pattern>org.apache.beam.runners</pattern> | ||
| <!--suppress MavenModelInspection --> | ||
| <shadedPattern> | ||
| org.apache.beam.runners.direct.util |
There was a problem hiding this comment.
*.util -> *.repackaged.runners.util
runners/direct-java/pom.xml
Outdated
| <plugin> | ||
| <groupId>org.apache.maven.plugins</groupId> | ||
| <artifactId>maven-shade-plugin</artifactId> | ||
| <version>3.0.0</version> |
dhalperi
left a comment
dhalperi
left a comment
There was a problem hiding this comment.
Post maven dependency:tree and comment on its goodness?
tgroh
force-pushed
the
direct_runner_deps
branch
3 times, most recently
from
d16024f to
8648b51
Compare
tgroh
force-pushed
the
direct_runner_deps
branch
3 times, most recently
from
6a477f0 to
b76481c
Compare
|
(Removing test and runtime dependencies) [INFO] --- maven-dependency-plugin:3.0.0:tree (default-cli) @ beam-runners-direct-java --- We shade and repackage guava, runners-core-construction-java and runners-core-java. It may also be appropriate to repackage the runner API. My belief is that all other dependencies are also dependencies of the Java SDK, and thus cannot be repackaged. |
dhalperi
left a comment
dhalperi
left a comment
There was a problem hiding this comment.
Is there not also transitive dependency on protobuf through common-runner-api? That would surprise me, but I don't see it in your list.
If proto's not in the dependency-reduced-pom.xml, I'm down.. but I'm not sure how it's not, exactly. (Unless shade does the transitive magic).
| public class DirectRunnerApiSurfaceTest { | ||
| @Test | ||
| public void testDirectRunnerApiSurface() throws Exception { | ||
| // The DirectRunner should expose no more than the Core SDK |
There was a problem hiding this comment.
looks like this comment is now out of date?
| @RunWith(JUnit4.class) | ||
| public class SdkCoreApiSurfaceTest { | ||
| @SuppressWarnings("unchecked") | ||
| public static final Set<String> ALLOWED_PACKAGES = |
There was a problem hiding this comment.
no longer needed in this PR?
|
Whoops, ran on the non-reduced POM. We don't have a direct dependency on proto; right now we get a transitive dependency via the core SDK and runners-core-construction (and probably runners-core). Those can (and probably should) be shaded independently [INFO] --- maven-dependency-plugin:3.0.0:tree (default-cli) @ beam-runners-direct-java --- |
|
Protobuf is shaded. |
|
R: @dhalperi |
|
retest this please |
|
The failure was in archetype generation, which appears unrelated. |
|
retest this please |
|
Changes Unknown when pulling f21a6b5 on tgroh:direct_runner_deps into ** on apache:master**. |
|
PTAL I needed to include the Runner API protos to shade |
When a worker dies because of an error, propagate that error and fail the Pipeline.
Add an API Surface Test
| // The DirectRunner can expose the Core SDK, anything exposed by the Core SDK, and itself | ||
| @SuppressWarnings("unchecked") | ||
| final Set<String> allowed = | ||
| ImmutableSet.of("org.apache.beam.sdk", "org.apache.beam.runners.direct", "org.joda.time"); |
There was a problem hiding this comment.
For future improvement, would be nice to make this be just specific classes Pipeline, PipelineOptions, PipelineRunner, and maybe that's roughly it.
|
LGTM, merging. please CP to release. |
4 participants
Be sure to do all of the following to help us incorporate your contribution
quickly and easily:
[BEAM-<Jira issue #>] Description of pull requestmvn clean verify. (Even better, enableTravis-CI on your fork and ensure the whole test matrix passes).
<Jira issue #>in the title with the actual Jira issuenumber, if there is one.
Individual Contributor License Agreement.
Move inner classes of the DirectRunner to reduce total API Surface.
Add API Surface Test for the Direct Runner.