New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add ability to encode and decode histogram data to portable runners #33013

Merged

johnjcasey merged 3 commits into apache:master from Naireen:add_hist_to_runner_implementation

Nov 15, 2024

Contributor

Naireen commented Nov 5, 2024 •

edited

Loading

Add ability to encode and decode histogram data to portable runners

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 bot added java runners core labels

Naireen force-pushed the add_hist_to_runner_implementation branch 4 times, most recently from 3a39975 to 3d01bec Compare

November 5, 2024 19:33

Contributor Author

Naireen commented Nov 5, 2024

Run Java_IOs_Direct PreCommit

Contributor Author

Naireen commented Nov 5, 2024

Run Java_Hadoop_IO_Direct PreCommit

Contributor Author

Naireen commented Nov 5, 2024

Run Java PreCommit

Contributor Author

Naireen commented Nov 5, 2024

Run Java_IOs_Direct PreCommit

Contributor Author

Naireen commented Nov 5, 2024

Run Java_GCP_IO_Direct PreCommit

Contributor Author

Naireen commented Nov 5, 2024

Run Java_IOs_Direct PreCommit

Contributor Author

Naireen commented Nov 5, 2024

Run Java PreCommit

Contributor Author

Naireen commented Nov 5, 2024

Run Java_IOs_Direct PreCommit

Naireen force-pushed the add_hist_to_runner_implementation branch from 3d01bec to 4f8b923 Compare

November 6, 2024 04:41

Contributor Author

Naireen commented Nov 6, 2024

Run Java_IOs_Direct PreCommit

Contributor Author

Naireen commented Nov 6, 2024

Run Java PreCommit

1 similar comment

Contributor Author

Naireen commented Nov 6, 2024

Run Java PreCommit

Naireen marked this pull request as ready for review

November 6, 2024 18:57

Contributor

github-actions bot commented Nov 6, 2024

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

Contributor Author

Naireen commented Nov 6, 2024

assign set of reviewers

Contributor

github-actions bot commented Nov 6, 2024

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

R: @m-trieu 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).

github-actions bot added the Next Action: Reviewers label

Naireen force-pushed the add_hist_to_runner_implementation branch 2 times, most recently from 11a08fa to 25a7cfd Compare

November 11, 2024 21:25

Contributor Author

Naireen commented Nov 11, 2024

Run Java_IOs_Direct PreCommit

Contributor Author

Naireen commented Nov 12, 2024

Run Java PreCommit

1 similar comment

Contributor Author

Naireen commented Nov 12, 2024

Run Java PreCommit

johnjcasey approved these changes

View reviewed changes

...rs/core-java/src/main/java/org/apache/beam/runners/core/metrics/MonitoringInfoEncodings.java Outdated Show resolved Hide resolved

Naireen mentioned this pull request

[Feature Request]: Refactor Histogram Implementation out of DF runner so it can be used by portable and other runners #33093

Open

17 tasks

Naireen force-pushed the add_hist_to_runner_implementation branch from 0982a45 to c9b7345 Compare

November 12, 2024 20:26

Naireen added 2 commits

November 12, 2024 20:27


          add ability to encode and decode histogram data to portable runners

21cc654

add ability to encode and decode histogram data to portable runners


          link github issue

24ce41c

Naireen force-pushed the add_hist_to_runner_implementation branch from c9b7345 to 24ce41c Compare

November 12, 2024 20:27

m-trieu reviewed

View reviewed changes

...rs/core-java/src/main/java/org/apache/beam/runners/core/metrics/MonitoringInfoEncodings.java Outdated

+                      base2Exp.setScale(buckets.getScale());
+                      outputHistogram2.setBucketOptions(new BucketOptions().setExponential(base2Exp));
+                    } else {
+                      throw new RuntimeException("Unable to parse histogram, bucket is not recognized");

Contributor

m-trieu Nov 13, 2024

Can we throw a more specific exception here?

It can extend RuntimeException but will help with debugging

Contributor

m-trieu Nov 13, 2024

Also what does the caller do if an exception is thrown?

Do we want to LOG the error and return Optional if we don't care about the exception?

Contributor Author

Naireen Nov 14, 2024

Done, returned a custom error message

m-trieu suggested changes

View reviewed changes

...rs/core-java/src/main/java/org/apache/beam/runners/core/metrics/MonitoringInfoEncodings.java

+                    return ByteString.copyFromUtf8(jsonString);
+                  } catch (Exception e) {
+                    throw new RuntimeException(e);

Contributor

m-trieu Nov 13, 2024

ditto above

Can we throw a more specific exception here?

It can extend RuntimeException but will help with debugging

Contributor Author

Naireen Nov 14, 2024

Done.

Contributor Author

Naireen Nov 14, 2024

To clarify, when encoding and decoding, if there are specific errors like the format isn't as expected (or if the bucket isn't supported), I raise the HistogramParsingException error, and then have the generic

catch (IOException e) {
throw new RuntimeException(e);

at the end that is a part of all encoding and decodings.

...rs/core-java/src/main/java/org/apache/beam/runners/core/metrics/MonitoringInfoEncodings.java Outdated

+                      base2Exp.setScale(expNode.get("scale").asInt());
+                      newHist.setBucketOptions(new BucketOptions().setExponential(base2Exp));
+                    } else {
+                      throw new RuntimeException("Unable to parse histogram, bucket is not recognized");

Contributor

m-trieu Nov 13, 2024

ditto above

Can we throw a more specific exception here?

It can extend RuntimeException but will help with debugging

Contributor

m-trieu Nov 13, 2024

Also what does the caller do if an exception is thrown?

Do we want to LOG the error and return Optional if we don't care about the exception?

Contributor Author

Naireen Nov 14, 2024

I followed the existing style, where it throws an error. Essentially nothing happens if an error is thrown (the call is here, where we dont actaully return anything:

beam/runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/MetricsContainerImpl.java

Line 518 in f38edb7

private void updateForLatestInt64Type(MonitoringInfo monitoringInfo) {

) This is separate than the data processing path, so if there is an error in the metrics processing, its not retried.
We can update this to return an optional for all the decoders instead if we want, but that should likely be a separate change and should ve done for all of them at once

...rs/core-java/src/main/java/org/apache/beam/runners/core/metrics/MonitoringInfoEncodings.java Outdated

+                      base2Exp.setScale(buckets.getScale());
+                      outputHistogram2.setBucketOptions(new BucketOptions().setExponential(base2Exp));
+                    } else {
+                      throw new RuntimeException("Unable to parse histogram, bucket is not recognized");

Contributor

m-trieu Nov 13, 2024

Also what does the caller do if an exception is thrown?

Do we want to LOG the error and return Optional if we don't care about the exception?

sdks/java/core/src/main/java/org/apache/beam/sdk/util/HistogramData.java Outdated Show resolved Hide resolved

sdks/java/core/src/main/java/org/apache/beam/sdk/util/HistogramData.java Show resolved Hide resolved

sdks/java/core/src/main/java/org/apache/beam/sdk/util/HistogramData.java Show resolved Hide resolved


          address comments

c9d4de9

Naireen force-pushed the add_hist_to_runner_implementation branch from 37ffd60 to c9d4de9 Compare

November 14, 2024 17:40

Contributor Author

Naireen commented Nov 14, 2024

Run Java_IOs_Direct PreCommit

Contributor Author

Naireen commented Nov 14, 2024

Run Java PreCommit

1 similar comment

Contributor Author

Naireen commented Nov 14, 2024

Run Java PreCommit

m-trieu approved these changes

View reviewed changes

Contributor

m-trieu left a comment

LGTM

Contributor

github-actions bot commented Nov 14, 2024

R: @robertwb for final approval

johnjcasey merged commit 5bfacc9 into apache:master

34 checks passed

kennknowles reviewed

View reviewed changes

sdks/java/core/build.gradle

@@ @@ -75,6 +75,7 @@ dependencies { @@
                 permitUnusedDeclared library.java.antlr
                 permitUsedUndeclared library.java.antlr_runtime
                 // Required to load constants from the model, e.g. max timestamp for global window
+                provided library.java.google_api_services_dataflow

Member

kennknowles Jan 24, 2025

This is a totally not allowed dependency. We have to revert this.

kennknowles reviewed

View reviewed changes

runners/core-java/build.gradle

@@ @@ -42,6 +42,7 @@ dependencies { @@
                 implementation project(path: ":model:pipeline", configuration: "shadow")
                 implementation project(path: ":sdks:java:core", configuration: "shadow")
                 implementation project(path: ":model:job-management", configuration: "shadow")
+                implementation library.java.google_api_services_dataflow

Member

kennknowles Jan 24, 2025

This module is also not allowed to depend on runner-specific things.

kennknowles reviewed

View reviewed changes

sdks/java/harness/build.gradle

@@ @@ -30,6 +30,7 @@ dependencies { @@
                 provided project(path: ":model:pipeline", configuration: "shadow")
                 provided project(path: ":sdks:java:core", configuration: "shadow")
                 provided project(path: ":sdks:java:transform-service:launcher")
+                provided library.java.google_api_services_dataflow

Member

kennknowles Jan 24, 2025

This module is also not allowed to depend on runner-specific things

kennknowles reviewed

View reviewed changes

sdks/java/core/src/main/java/org/apache/beam/sdk/util/HistogramData.java

+                 *
+                 * @param histogramProto DataflowHistogramValue proto used to populate stats for the histogram.
+                 */
+                public HistogramData(DataflowHistogramValue histogramProto) {

Member

kennknowles Jan 24, 2025

This seems like Dataflow utility code that could live in runners/dataflow/worker/ somewhere

Member

kennknowles commented Jan 24, 2025

This made it into 2.62.0. It may not cause user issues so we don't need to publish a known issue report yet. But we need to revert it and probably get a forward fix in for 2.63.0 to avoid backwards incompatible breakage @jrmccluskey

kennknowles added a commit to kennknowles/beam that referenced this pull request


          Revert "Add ability to encode and decode histogram data to portable r…

70dda27

…unners (apache#33013)"

This reverts commit 5bfacc9. The change caused the
core SDK, the SDK harness, and the shared runners-core library to depend on Dataflow.

Member

kennknowles commented Jan 24, 2025

To be clear - this is not a philosophical point about runner independence, but an architectural concern. I've opened #33761 and then you can go forward again with a change that doesn't touch dependencies.

kennknowles added a commit to kennknowles/beam that referenced this pull request


          Revert "Add ability to encode and decode histogram data to portable r…

03da258

…unners (apache#33013)"

This reverts commit 5bfacc9. The change caused the
core SDK, the SDK harness, and the shared runners-core library to depend on Dataflow.

kennknowles added a commit that referenced this pull request


          Merge pull request #33761: Revert "Add ability to encode and decode h…

ad006a8

…istogram data to portable runners (#33013)"

Abacn pushed a commit that referenced this pull request


          Revert "Add ability to encode and decode histogram data to portable r…

6ee5a50

…unners (#33013)"

This reverts commit 5bfacc9. The change caused the
core SDK, the SDK harness, and the shared runners-core library to depend on Dataflow.

tomstepp pushed a commit to tomstepp/apache-beam that referenced this pull request


          Revert "Add ability to encode and decode histogram data to portable r…

783ff06

…unners (apache#33013)"

This reverts commit 5bfacc9. The change caused the
core SDK, the SDK harness, and the shared runners-core library to depend on Dataflow.

VardhanThigle pushed a commit to VardhanThigle/beam that referenced this pull request


          Revert "Add ability to encode and decode histogram data to portable r…

a1a7f47

…unners (apache#33013)"

This reverts commit 5bfacc9. The change caused the
core SDK, the SDK harness, and the shared runners-core library to depend on Dataflow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core java Next Action: Reviewers runners