Merge runners-core-construction into sdks-java-core #29924

kennknowles · 2024-01-04T21:26:34Z

This is a large-scale change to eliminate the toil of making Java transforms available as portable transforms. This brings the SDK in line with Python and Go, with the protobuf dependencies and core features being simply part of the main module.

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

Mention the appropriate issue in your description (for example: addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment fixes #<ISSUE NUMBER> instead.
Update CHANGES.md with noteworthy changes.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md

GitHub Actions Tests Status (on master branch)

See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.

github-actions · 2024-01-06T01:53:18Z

Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment assign set of reviewers

kennknowles · 2024-02-12T15:21:45Z

@chamikaramj @Abacn this one is now green, too. It includes #30235. But I think it may be better to merge one at a time to take it slow.

kennknowles · 2024-02-13T18:24:07Z

The result of the classpath scanning on windows seems to be stable in red. I will try to investigate.

kennknowles · 2024-02-13T18:24:58Z

It does not seem to upload a scan so I cannot get the details :-(

chamikaramj

Thanks!

chamikaramj · 2024-02-13T20:45:10Z

runners/core-construction-java/build.gradle

- * limitations under the License.
- */
-
-plugins { id 'org.apache.beam.module' }


chamikaramj · 2024-02-13T21:03:29Z

runners/direct-java/build.gradle

                        ":runners:core-java",
                        ":runners:local-java",
                        ":runners:java-fn-execution",
-                        ":sdks:java:extensions:avro"


Avro dependency is not needed anymore ?

chamikaramj · 2024-02-13T21:06:34Z

runners/direct-java/build.gradle

@@ -67,7 +66,6 @@ dependencies {
  dependOnProjects.each {
    implementation project(it)
  }
-  shadow project(path: ":sdks:java:core", configuration: "shadow")


Ditto. Why did direct-java needed to depend on "core" before ?

Now it depends on core above, in the dependOnProjects section.

chamikaramj · 2024-02-13T21:43:43Z

sdks/java/core/build.gradle

@@ -24,6 +24,19 @@ applyJavaNature(
    'FileIO': 'https://github.com/typetools/checker-framework/issues/6388',
    'MergingActiveWindowSetTest': 'https://github.com/typetools/checker-framework/issues/3776',
    'WindowFnTestUtils': 'https://github.com/typetools/checker-framework/issues/3776',
+    'WindowIntoTranslation': 'https://github.com/typetools/checker-framework/issues/3791',


typetools/checker-framework#3791 has been fixed.

Should we reference a different bug here ?

I did a checkerframework upgrade but didn't update this list. Seeing if these no longer crash now.

Done. I had to add suppression to each file but they don't crash checkerframework.

chamikaramj · 2024-02-13T21:45:17Z

sdks/java/core/build.gradle

@@ -77,13 +90,16 @@ dependencies {
  shadow project(path: ":model:pipeline", configuration: "shadow")
  shadow project(path: ":model:fn-execution", configuration: "shadow")
  shadow project(path: ":model:job-management", configuration: "shadow")
+  shadow project(path: ":model:fn-execution", configuration: "shadow")
+  shadow project(path: ":sdks:java:transform-service:launcher") // not "shadow" configuration, which is an uberjar


Why do we need the "shadow" configuration for other projects referenced above ?

Note that after https://github.com/apache/beam/pull/30190/files#diff-c85538d3151bb7ac1847f9b888558c84a82705624f6fc453648aba1fa135c6d8 this is not an uber jar anymore.

Ah, I'll try it. We may not need the shadow configurations of the other projects either. It is confusing and changes based on whether the shadow plugin is used in a project.

Done. There is no shadow configuration for the launcher. But for the other ones, the non-shadow configuration is an uber jar (or at least it was). I think the solution will be to start at the leaves and remove extraneous use of shadow plugin. A big project and not as urgent I think.

chamikaramj · 2024-02-13T21:48:19Z

sdks/java/extensions/python/build.gradle

    implementation project(path: ":sdks:java:core", configuration: "shadow")
-    implementation project(path: ":sdks:java:transform-service:launcher")
+    implementation project(path: ":sdks:java:transform-service:launcher") // not "shadow" configuration, which is an uberjar


Ditto regarding the inconsistency when depending with other projects vs this one.

Done. It turns out launcher doesn't have a shadow configuration anyhow. This is turned on/off by whether the shadow plugin is active. Ideally we stop using it, because vendoring is the only shading we need, really.

Abacn · 2024-02-14T01:39:39Z

What did core-construction-java originally do? I see it moved to "org.apache.beam.sdk.util.construction", Is it a utility? Thinking about this module or "org.apache.beam.sdk.construction" looks better. As a similar module "org.apache.beam.sdk.expansion" just has its top level namespace after sdk.

kennknowles · 2024-02-14T01:52:52Z

What did core-construction-java originally do? I see it moved to "org.apache.beam.sdk.util.construction", Is it a utility? Thinking about this module or "org.apache.beam.sdk.construction" looks better. As a similar module "org.apache.beam.sdk.expansion" just has its top level namespace after sdk.

@Abacn the util folder in the SDK is "not for users" and has its javadoc hidden. It is kind of a hacky way but it has a long history.

The history of core construction is a few different things:

runners/core-java was created for execution helpers for runners, so they could all build their workers.
later we created transform overrides that are for runners, but only before job submission and I think that is when runners/core-construction-java was created. Specifically, things like DataflowRunner were allowed to depend on core-construction-java but they were not allowed to depend on core-java. Only the worker is allowed to depend on core-java.
The really big reason was to keep proto and gRPC dependencies out of the core SDK. We wanted the core SDK to be "pure" for each language, and portability would be an optional plugin/add-on. Now we have a totally different philosophy where portability comes first and SDKs come second, and everything depends on proto so much there is no point.

I realize now that actually I could have just added PTransform.toProto to the Java SDK without doing this merge 😅 and just deleted the translation from core-construction-java.

But to summarize my intention with all these changes, it is to make it much much easier to add portability stuff to every transform in the Java SDK, but just implementing PTransform.getURN() and PTransform.toProto() basically. Get rid of all the @AutoService stuff and all the interfaces involved there.

kennknowles · 2024-02-14T18:13:12Z

green!

Abacn

Thanks for the detailed explanation. Just had a comment

After these refactors, good to add an item in CHANGES.md

Abacn · 2024-02-14T19:12:24Z

sdks/java/core/build.gradle

@@ -77,13 +77,16 @@ dependencies {
  shadow project(path: ":model:pipeline", configuration: "shadow")
  shadow project(path: ":model:fn-execution", configuration: "shadow")
  shadow project(path: ":model:job-management", configuration: "shadow")
+  shadow project(path: ":model:fn-execution", configuration: "shadow")


duplicate ":model:fn-execution"

Thanks! Fixed

chamikaramj

Thanks. LGTM.

…construction-java

kennknowles · 2024-02-14T20:51:32Z

Added CHANGES commit to the PR and I'll merge them together once green again. (no code changes but it is nice to see green in the history)

Abacn

Thanks!

Abacn · 2024-02-20T16:26:44Z

buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy

@@ -2748,11 +2746,9 @@ class BeamModulePlugin implements Plugin<Project> {
          systemProperty "expansionPort", port
          systemProperty "semiPersistDir", config.semiPersistDir
          classpath = config.classpath + project.files(
-              project.project(":runners:core-construction-java").sourceSets.test.runtimeClasspath,


We still need them, otherwise XVR javaUsingPython not run the tests, see #30353

honnix · 2024-04-24T14:24:35Z

CHANGES.md

@@ -67,6 +67,8 @@

 * [Enrichment Transform](https://s.apache.org/enrichment-transform) along with GCP BigTable handler added to Python SDK ([#30001](https://github.com/apache/beam/pull/30001)).
 * Allow writing clustered and not time partitioned BigQuery tables (Java) ([#30094](https://github.com/apache/beam/pull/30094)).
+* Merged sdks/java/fn-execution and runners/core-construction-java into the main SDK. These artifacts were never meant for users, but noting


This is unfortunately an unpleasant breaking change. We don't have much context why core-construction-java was never meant for users since we use some of the classes. This change makes it impossible for us to rollout to our users.

More concretely we have a customized implementation of PipelineResourcesDetector, among a few other references to classes in core-construction-java.

We will appreciate at least this was mentioned in the release notes.

github-actions bot added python java website io extensions runners spark dataflow direct aws gcp kafka core flink fn-execution jet portability samza twister2 labels Jan 4, 2024

kennknowles force-pushed the runners-core-construction branch from 4f3a5d6 to 5433c8e Compare January 4, 2024 21:28

kennknowles mentioned this pull request Jan 5, 2024

Re-enable checker for files excluded to due bug (now fixed) #29932

Closed

3 tasks

kennknowles force-pushed the runners-core-construction branch 6 times, most recently from f49e88c to 5a7404c Compare January 6, 2024 00:21

kennknowles marked this pull request as ready for review January 6, 2024 00:29

kennknowles force-pushed the runners-core-construction branch from 5a7404c to 2bc53e0 Compare January 8, 2024 20:51

This was referenced Feb 6, 2024

Remove avro dependency from runners-core-construction #30235

Merged

Adjust transform-service module names and deps with sdks/java/core #30237

Merged

kennknowles force-pushed the runners-core-construction branch 3 times, most recently from 1a6438a to 4486b62 Compare February 11, 2024 17:32

kennknowles force-pushed the runners-core-construction branch from 4486b62 to 1b7a11c Compare February 13, 2024 15:30

chamikaramj reviewed Feb 13, 2024

View reviewed changes

Use canonical path when testing classpath scanner

eef267f

kennknowles force-pushed the runners-core-construction branch from fa9d709 to cdf1af6 Compare February 14, 2024 01:39

Abacn reviewed Feb 14, 2024

View reviewed changes

chamikaramj approved these changes Feb 14, 2024

View reviewed changes

kennknowles added 2 commits February 14, 2024 15:48

Merge runners-core-construction into sdks-java-core

f73ce82

CHANGES note: elimination of sdks/java/fn-execution and runners/core-…

7c7820e

…construction-java

kennknowles force-pushed the runners-core-construction branch from cdf1af6 to 7c7820e Compare February 14, 2024 20:51

Abacn approved these changes Feb 14, 2024

View reviewed changes

kennknowles merged commit 53c966d into apache:master Feb 15, 2024
38 checks passed

kennknowles deleted the runners-core-construction branch February 15, 2024 15:45

ahmedabu98 mentioned this pull request Feb 16, 2024

Fix Python postcommits #30333

Merged

Abacn mentioned this pull request Feb 20, 2024

[Failing Test]: XVR tests running on VR test suites causing test failure #30353

Closed

16 tasks

Abacn reviewed Feb 20, 2024

View reviewed changes

Abacn mentioned this pull request Mar 12, 2024

[Bug]: hamcrest required in class path in 2.55.0 #30617

Closed

16 tasks

Polber mentioned this pull request Mar 27, 2024

[Bug]: Beam YAML WriteToJson fails on Beam 2.55 #30776

Closed

16 tasks

honnix reviewed Apr 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge runners-core-construction into sdks-java-core #29924

Merge runners-core-construction into sdks-java-core #29924

kennknowles commented Jan 4, 2024 •

edited

Loading

github-actions bot commented Jan 6, 2024

kennknowles commented Feb 12, 2024

kennknowles commented Feb 13, 2024

kennknowles commented Feb 13, 2024

chamikaramj left a comment

chamikaramj Feb 13, 2024

chamikaramj Feb 13, 2024

kennknowles Feb 14, 2024

chamikaramj Feb 13, 2024

kennknowles Feb 14, 2024

chamikaramj Feb 13, 2024

kennknowles Feb 14, 2024

kennknowles Feb 14, 2024

chamikaramj Feb 13, 2024

kennknowles Feb 14, 2024

kennknowles Feb 14, 2024

chamikaramj Feb 13, 2024

kennknowles Feb 14, 2024

Abacn commented Feb 14, 2024

kennknowles commented Feb 14, 2024

kennknowles commented Feb 14, 2024

Abacn left a comment

Abacn Feb 14, 2024

kennknowles Feb 14, 2024

chamikaramj left a comment

kennknowles commented Feb 14, 2024

Abacn left a comment

Abacn Feb 20, 2024

honnix Apr 24, 2024

honnix Apr 24, 2024

honnix Apr 24, 2024

Merge runners-core-construction into sdks-java-core #29924

Merge runners-core-construction into sdks-java-core #29924

Conversation

kennknowles commented Jan 4, 2024 • edited Loading

GitHub Actions Tests Status (on master branch)

github-actions bot commented Jan 6, 2024

kennknowles commented Feb 12, 2024

kennknowles commented Feb 13, 2024

kennknowles commented Feb 13, 2024

chamikaramj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Abacn commented Feb 14, 2024

kennknowles commented Feb 14, 2024

kennknowles commented Feb 14, 2024

Abacn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chamikaramj left a comment

Choose a reason for hiding this comment

kennknowles commented Feb 14, 2024

Abacn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kennknowles commented Jan 4, 2024 •

edited

Loading