Parse YAML ExpansionService configs directly using SnakeYAML #31406

chamikaramj · 2024-05-25T01:58:00Z

This fixes #31405.

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-05-25T08:06:07Z

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

chamikaramj · 2024-05-25T14:57:26Z

assign set of reviewers

github-actions · 2024-05-25T14:58:32Z

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

R: @kennknowles for label java.

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).

chamikaramj · 2024-05-27T04:42:49Z

Failures don't seem to be related.

chamikaramj · 2024-05-29T23:05:31Z

@kennknowles @Abacn friendly ping since I'd like to get this into the 2.57.0 release.

kennknowles · 2024-05-31T17:23:00Z

sdks/java/expansion-service/build.gradle

@@ -41,10 +41,8 @@ dependencies {
  implementation project(path: ":sdks:java:core", configuration: "shadow")
  implementation project(path: ":runners:java-fn-execution")
  implementation project(path: ":sdks:java:harness")
+  implementation "org.yaml:snakeyaml:2.0"


put this in the BeamModulePlugin?

Had to revert, pls see the top level comment.

Moved back to the top level.

kennknowles · 2024-05-31T17:25:00Z

...ion-service/src/main/java/org/apache/beam/sdk/expansion/service/ExpansionServiceOptions.java

+        try (InputStream stream = new FileInputStream(allowListFileObj)) {
+          return AllowList.parseFromYamlStream(stream);
+        } catch (FileNotFoundException e) {
+          throw new RuntimeException(e);


Would be nice to add a message to these RuntimeException wrappers, like "when opening file to parse as ExpansionServiceConfig"

kennknowles · 2024-05-31T17:25:20Z

...ion-service/src/main/java/org/apache/beam/sdk/expansion/service/ExpansionServiceOptions.java

        File configFileObj = new File(configFile);
        if (!configFileObj.exists()) {
          throw new IllegalArgumentException("Config file " + configFile + " does not exist");
        }
-        try {
-          return mapper.readValue(configFileObj, ExpansionServiceConfig.class);
+        try (InputStream stream = new FileInputStream(configFileObj)) {


Here too, would be nice to add a message to these RuntimeException wrappers, like "when opening file to parse as ExpansionServiceConfig"

kennknowles · 2024-05-31T17:26:33Z

...ce/src/main/java/org/apache/beam/sdk/expansion/service/JavaClassLookupTransformProvider.java

+      Yaml yaml = new Yaml();
+      Map<Object, Object> config = yaml.load(inputStream);
+
+      if (config != null) {


Readability: Reverse the if statement and make it if (config == null) throw IllegalArgumentException

kennknowles · 2024-05-31T17:26:56Z

...ce/src/main/java/org/apache/beam/sdk/expansion/service/JavaClassLookupTransformProvider.java

    public abstract String getVersion();

+    @SuppressWarnings("mutable")


Would making it ImmutableList be just as good as suppression?

I added these since I got some random warnings when compiling but seems like this didn't fully resolve the issue. So removing these suppressions for now.

chamikaramj

Thanks. PTAL.

chamikaramj · 2024-05-31T19:15:50Z

sdks/java/expansion-service/build.gradle

@@ -41,10 +41,8 @@ dependencies {
  implementation project(path: ":sdks:java:core", configuration: "shadow")
  implementation project(path: ":runners:java-fn-execution")
  implementation project(path: ":sdks:java:harness")
+  implementation "org.yaml:snakeyaml:2.0"


chamikaramj · 2024-05-31T19:20:43Z

...ion-service/src/main/java/org/apache/beam/sdk/expansion/service/ExpansionServiceOptions.java

+        try (InputStream stream = new FileInputStream(allowListFileObj)) {
+          return AllowList.parseFromYamlStream(stream);
+        } catch (FileNotFoundException e) {
+          throw new RuntimeException(e);


chamikaramj · 2024-05-31T19:20:50Z

...ion-service/src/main/java/org/apache/beam/sdk/expansion/service/ExpansionServiceOptions.java

        File configFileObj = new File(configFile);
        if (!configFileObj.exists()) {
          throw new IllegalArgumentException("Config file " + configFile + " does not exist");
        }
-        try {
-          return mapper.readValue(configFileObj, ExpansionServiceConfig.class);
+        try (InputStream stream = new FileInputStream(configFileObj)) {


chamikaramj · 2024-05-31T19:23:13Z

...ce/src/main/java/org/apache/beam/sdk/expansion/service/JavaClassLookupTransformProvider.java

+      Yaml yaml = new Yaml();
+      Map<Object, Object> config = yaml.load(inputStream);
+
+      if (config != null) {


chamikaramj · 2024-05-31T19:30:02Z

...ce/src/main/java/org/apache/beam/sdk/expansion/service/JavaClassLookupTransformProvider.java

    public abstract String getVersion();

+    @SuppressWarnings("mutable")


I added these since I got some random warnings when compiling but seems like this didn't fully resolve the issue. So removing these suppressions for now.

chamikaramj · 2024-05-31T21:32:45Z

Seems like this results in a test failure. Looking.

chamikaramj · 2024-05-31T21:56:12Z

Seems like simply defining SnakeYAML at top level causes "./gradlew :runners:java-fn-execution:test" to fail due to the conflict [1]. So reverted that and defining the dependency at the sub-module level now.

[1] #26743 (comment)

kennknowles · 2024-06-01T00:40:05Z

Seems like simply defining SnakeYAML at top level causes "./gradlew :runners:java-fn-execution:test" to fail due to the conflict [1]. So reverted that and defining the dependency at the sub-module level now.

[1] #26743 (comment)

Oh I just meant defining library.java.snakeyaml = <string for dep> so it should be just an alias. That alone should be a noop for the actual deps, just a way of factoring the gradle files. Is there a conflict in that case? Do we have another module that depends on snakeyaml at a different version? Or it sounds like we just can't use snakeyaml at all because of GCP dependency on Jackson?

chamikaramj · 2024-06-01T01:30:02Z

Yeah, there's an actual conflict tracked here: #26743

This PR was a workaround.

chamikaramj · 2024-06-03T01:15:56Z

This is not a release blocker anymore since #31473 was merged but I think it's still good to get this in to simplify by removing the Jackson dependency from the Expansion Service.

PTAL.

kennknowles

LGTM when green

chamikaramj · 2024-06-03T20:12:34Z

Thanks.

Java pre-commit passed. Two other failures seems to be unrelated and just flake test suites.

Abacn · 2024-06-03T20:23:25Z

Hadoop IO Direct failure is due to this change. It's passing on master branch: https://github.com/apache/beam/actions/workflows/beam_PreCommit_Java_Hadoop_IO_Direct.yml?query=event%3Aschedule

The cause is snakeyaml version change: snakeyaml 2.x is not compatible with cassandra-all used in the test

I tried to pin snakeyaml version in #31473 but here more beam component introduced snakeyaml 2.x dependency and the pinned dependency appearently got overwritten

chamikaramj · 2024-06-03T20:29:23Z

Hmm, this is just bumping the existing dependency in core from 2.0 to 2.2. Lemme try some tests. Feel free to send a revert if needed.

chamikaramj · 2024-06-03T22:30:07Z

Seems like #31485 fixes the issue.

github-actions bot added the java label May 25, 2024

github-actions bot added the Next Action: Reviewers label May 25, 2024

chamikaramj added this to the 2.57.0 Release milestone May 28, 2024

chamikaramj requested a review from Abacn May 28, 2024 20:10

kennknowles reviewed May 31, 2024

View reviewed changes

chamikaramj force-pushed the expansion_service_upgrade_yaml_config branch from fa1c9b7 to 4e9f30c Compare May 31, 2024 19:39

github-actions bot added build and removed build labels May 31, 2024

chamikaramj commented May 31, 2024

View reviewed changes

chamikaramj force-pushed the expansion_service_upgrade_yaml_config branch from 4e9f30c to 27c70b8 Compare May 31, 2024 20:01

github-actions bot added build and removed build labels May 31, 2024

chamikaramj force-pushed the expansion_service_upgrade_yaml_config branch from 27c70b8 to 3f024f5 Compare May 31, 2024 21:53

chamikaramj added 4 commits June 2, 2024 11:15

Parse YAML ExpansionService configs directly using SnakeYAML

831b268

Addressing reviewer comments

3866f2f

Revert defining SnakeYaml at top level

f32522f

Moves the dependency definition to the top level

eba6ef0

chamikaramj force-pushed the expansion_service_upgrade_yaml_config branch from 3f024f5 to eba6ef0 Compare June 2, 2024 18:36

github-actions bot added build and removed build labels Jun 2, 2024

kennknowles approved these changes Jun 3, 2024

View reviewed changes

chamikaramj merged commit 4ad7037 into apache:master Jun 3, 2024
27 of 30 checks passed

chamikaramj mentioned this pull request Jun 3, 2024

Pins SnakeYAML version to 1.33 for Hadoop Format tests #31485

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parse YAML ExpansionService configs directly using SnakeYAML #31406

Parse YAML ExpansionService configs directly using SnakeYAML #31406

chamikaramj commented May 25, 2024

github-actions bot commented May 25, 2024

chamikaramj commented May 25, 2024

github-actions bot commented May 25, 2024

chamikaramj commented May 27, 2024

chamikaramj commented May 29, 2024

kennknowles May 31, 2024

chamikaramj May 31, 2024

chamikaramj May 31, 2024

chamikaramj Jun 3, 2024

kennknowles May 31, 2024

chamikaramj May 31, 2024

kennknowles May 31, 2024

chamikaramj May 31, 2024

kennknowles May 31, 2024

chamikaramj May 31, 2024

kennknowles May 31, 2024

chamikaramj May 31, 2024

chamikaramj left a comment

chamikaramj May 31, 2024

chamikaramj May 31, 2024

chamikaramj May 31, 2024

chamikaramj May 31, 2024

chamikaramj May 31, 2024

chamikaramj commented May 31, 2024

chamikaramj commented May 31, 2024 •

edited

Loading

kennknowles commented Jun 1, 2024

chamikaramj commented Jun 1, 2024

chamikaramj commented Jun 3, 2024

kennknowles left a comment

chamikaramj commented Jun 3, 2024

Abacn commented Jun 3, 2024 •

edited

Loading

chamikaramj commented Jun 3, 2024

chamikaramj commented Jun 3, 2024

		public abstract String getVersion();

		@SuppressWarnings("mutable")

Parse YAML ExpansionService configs directly using SnakeYAML #31406

Parse YAML ExpansionService configs directly using SnakeYAML #31406

Conversation

chamikaramj commented May 25, 2024

GitHub Actions Tests Status (on master branch)

github-actions bot commented May 25, 2024

chamikaramj commented May 25, 2024

github-actions bot commented May 25, 2024

chamikaramj commented May 27, 2024

chamikaramj commented May 29, 2024

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

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

chamikaramj commented May 31, 2024

chamikaramj commented May 31, 2024 • edited Loading

kennknowles commented Jun 1, 2024

chamikaramj commented Jun 1, 2024

chamikaramj commented Jun 3, 2024

kennknowles left a comment

Choose a reason for hiding this comment

chamikaramj commented Jun 3, 2024

Abacn commented Jun 3, 2024 • edited Loading

chamikaramj commented Jun 3, 2024

chamikaramj commented Jun 3, 2024

chamikaramj commented May 31, 2024 •

edited

Loading

Abacn commented Jun 3, 2024 •

edited

Loading