[#25582] Use a Pathing Jar instead of long command line class paths in Java Boot loader. #26087

lostluck · 2023-04-04T03:23:12Z

Jars are recursive! If the manifest of one contains a manifest with a class path attribute, the JVM will load those jars. This gets around long jar sequences that can make an argument list too long.

This PR fixes an unchanged test in the Java boot_test.go, as well as adds a unit test for the pathing jar. Further, adds the container tests to the Go Tests github action, so this error doesn't recur.

Fixes #25582.

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.

lostluck · 2023-04-04T03:32:28Z

Run Java PostCommit

lostluck · 2023-04-04T03:34:32Z

Run Java examples on Dataflow Java 11

lostluck · 2023-04-04T03:34:58Z

Run Java Flink PortableValidatesRunner Streaming

lostluck · 2023-04-04T03:35:28Z

Run Dataflow ValidatesRunner

lostluck · 2023-04-04T03:35:53Z

Run Java Dataflow V2 ValidatesRunner

lostluck · 2023-04-04T03:55:41Z

Run Java 11 Examples on Dataflow Runner V2

lostluck · 2023-04-04T03:57:11Z

Run Java Examples on Dataflow Runner V2

lostluck · 2023-04-04T03:58:07Z

Run Java Spark PortableValidatesRunner Batch

github-actions · 2023-04-04T04:04:42Z

Assigning reviewers. If you would like to opt out of this review, comment assign to next reviewer:

R: @kennknowles for label java.
R: @damccorm for label build.

Available commands:

stop reviewer notifications - opt out of the automated review tooling
remind me after tests pass - tag the comment author after tests pass
waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

The PR bot will only process comments in the main thread (not review comments).

damccorm

LGTM

damccorm · 2023-04-04T13:41:19Z

.github/workflows/go_tests.yml

@@ -48,7 +48,7 @@ jobs:
      - name: Delete old coverage
        run: "cd sdks/go/pkg && rm -rf .coverage || :"
      - name: Run coverage
-        run: cd sdks/go/pkg && go test -coverprofile=coverage.txt -covermode=atomic ./...
+        run: cd sdks && go test -coverprofile=coverage.txt -covermode=atomic ./go/pkg/... ./go/container/... ./java/container/... ./python/container/... ./typescript/container/...


Question: does codecov count uncovered non-go files (e.g. https://github.com/apache/beam/blob/master/sdks/java/container/license_scripts/pull_licenses_java.py)? I'm wondering if it makes sense to run container tests separately outside of the context of codecov (I'm leaning towards thinking that they should be included so this is probably a no-op).

It will ignore those files, because it's unaware of files that don't receive any coverage numbers/changes.

I think we also don't have the covbot enabled for these directories either. But I'm less concerned about that than I am about the unit tests being run at all.

Enabling covbot here would be out of scope for this change I think. The go tests is different because it's otherwise hard to ensure an action will run, without having a change that ensures the action runs and that was fixing a clear gap.

SGTM - was meant to be nonblocking, but I realize now I didn't specify that

bvolpato · 2023-04-04T19:45:40Z

sdks/java/container/pathingjar.go

+	zf.Write([]byte("Manifest-Version: 1.0\n"))
+	zf.Write([]byte("Class-Path: " + strings.Join(classpaths, " ")))


nit: We could use Created-By: xyz to be able to trace back from a JAR to this pathing code

lostluck · 2023-04-04T20:13:32Z

Run Java Dataflow V2 ValidatesRunner

lostluck · 2023-04-04T20:13:38Z

Run Java Spark PortableValidatesRunner Batch

lostluck · 2023-04-04T20:13:47Z

Run Java Examples on Dataflow Runner V2

lostluck · 2023-04-04T20:14:13Z

Run Java Flink PortableValidatesRunner Streaming

lostluck · 2023-04-04T20:14:21Z

Run Java PostCommit

lostluck · 2023-04-07T19:54:49Z

Run Java Examples on Dataflow Runner V2

lostluck · 2023-04-07T19:55:06Z

Run Java Flink PortableValidatesRunner Streaming

lostluck · 2023-04-07T19:55:13Z

Run Java Dataflow V2 ValidatesRunner

lostluck · 2023-04-07T19:55:21Z

Run Java Spark PortableValidatesRunner Batch

lostluck · 2023-04-07T19:57:03Z

I had misunderstood something in my earlier research: The classpath appends are URIs, not file paths. URIs are in a format like :. In this case since it should be all jar files, the class paths should be prefixed with file:.

lostluck · 2023-04-07T20:06:39Z

Run Spotless PreCommit

lostluck · 2023-04-11T16:40:41Z

Apparently line lengths can't exceed 72 bytes. The more you know! Not hard to fix though (and validate in testing).

And I was missing some line breaks.

lostluck · 2023-04-11T17:37:10Z

Run Java Examples on Dataflow Runner V2

lostluck · 2023-04-11T18:16:44Z

Run Java Flink PortableValidatesRunner Streaming

lostluck · 2023-04-11T18:17:19Z

Run Java Spark PortableValidatesRunner Batch

lostluck · 2023-04-11T18:17:34Z

Run Java Dataflow V2 ValidatesRunner

lostluck · 2023-04-11T18:18:54Z

Run Java PostCommit

lostluck added 2 commits April 2, 2023 15:51

Use a Pathing Jar instead of long class paths.

4942059

Ensure boot.go container changes trigger go tests action.

f6a3d72

github-actions bot added build docker java labels Apr 4, 2023

lostluck changed the title ~~[#25582] Use a Pathing Jar instead of long class paths in Java Boot loader.~~ [#25582] Use a Pathing Jar instead of long command line class paths in Java Boot loader. Apr 4, 2023

github-actions bot added the Next Action: Reviewers label Apr 4, 2023

damccorm approved these changes Apr 4, 2023

View reviewed changes

bvolpato reviewed Apr 4, 2023

View reviewed changes

bvolpato approved these changes Apr 4, 2023

View reviewed changes

lostluck added 2 commits April 4, 2023 13:09

Add comments for pathingjar.

7ffa7c2

Only set classpath once.

fb2c1c1

lostluck added 2 commits April 7, 2023 12:53

URIs are a prefixed scheme.

18b4af5

Add comment about URIs

3d0a61d

Correct max line length length.

4b35162

lostluck merged commit e0e1398 into apache:master Apr 11, 2023

lostluck mentioned this pull request May 25, 2023

Fix Go Code Coverage. #26885

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#25582] Use a Pathing Jar instead of long command line class paths in Java Boot loader. #26087

[#25582] Use a Pathing Jar instead of long command line class paths in Java Boot loader. #26087

lostluck commented Apr 4, 2023

lostluck commented Apr 4, 2023

lostluck commented Apr 4, 2023

lostluck commented Apr 4, 2023

lostluck commented Apr 4, 2023

lostluck commented Apr 4, 2023

lostluck commented Apr 4, 2023

lostluck commented Apr 4, 2023

lostluck commented Apr 4, 2023

github-actions bot commented Apr 4, 2023

damccorm left a comment

damccorm Apr 4, 2023

lostluck Apr 4, 2023

lostluck Apr 4, 2023 •

edited

Loading

damccorm Apr 4, 2023

bvolpato Apr 4, 2023

lostluck Apr 4, 2023

lostluck commented Apr 4, 2023

lostluck commented Apr 4, 2023

lostluck commented Apr 4, 2023

lostluck commented Apr 4, 2023

lostluck commented Apr 4, 2023

lostluck commented Apr 7, 2023

lostluck commented Apr 7, 2023

lostluck commented Apr 7, 2023

lostluck commented Apr 7, 2023

lostluck commented Apr 7, 2023

lostluck commented Apr 7, 2023

lostluck commented Apr 11, 2023 •

edited

Loading

lostluck commented Apr 11, 2023

lostluck commented Apr 11, 2023

lostluck commented Apr 11, 2023

lostluck commented Apr 11, 2023

lostluck commented Apr 11, 2023

		zf.Write([]byte("Manifest-Version: 1.0\n"))
		zf.Write([]byte("Class-Path: " + strings.Join(classpaths, " ")))

[#25582] Use a Pathing Jar instead of long command line class paths in Java Boot loader. #26087

[#25582] Use a Pathing Jar instead of long command line class paths in Java Boot loader. #26087

Conversation

lostluck commented Apr 4, 2023

GitHub Actions Tests Status (on master branch)

lostluck commented Apr 4, 2023

lostluck commented Apr 4, 2023

lostluck commented Apr 4, 2023

lostluck commented Apr 4, 2023

lostluck commented Apr 4, 2023

lostluck commented Apr 4, 2023

lostluck commented Apr 4, 2023

lostluck commented Apr 4, 2023

github-actions bot commented Apr 4, 2023

damccorm left a comment

Choose a reason for hiding this comment

damccorm Apr 4, 2023

Choose a reason for hiding this comment

lostluck Apr 4, 2023

Choose a reason for hiding this comment

lostluck Apr 4, 2023 • edited Loading

Choose a reason for hiding this comment

damccorm Apr 4, 2023

Choose a reason for hiding this comment

bvolpato Apr 4, 2023

Choose a reason for hiding this comment

lostluck Apr 4, 2023

Choose a reason for hiding this comment

lostluck commented Apr 4, 2023

lostluck commented Apr 4, 2023

lostluck commented Apr 4, 2023

lostluck commented Apr 4, 2023

lostluck commented Apr 4, 2023

lostluck commented Apr 7, 2023

lostluck commented Apr 7, 2023

lostluck commented Apr 7, 2023

lostluck commented Apr 7, 2023

lostluck commented Apr 7, 2023

lostluck commented Apr 7, 2023

lostluck commented Apr 11, 2023 • edited Loading

lostluck commented Apr 11, 2023

lostluck commented Apr 11, 2023

lostluck commented Apr 11, 2023

lostluck commented Apr 11, 2023

lostluck commented Apr 11, 2023

lostluck Apr 4, 2023 •

edited

Loading

lostluck commented Apr 11, 2023 •

edited

Loading