Skip to content
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.

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

[BEAM-9110] Bump jboss-marshalling to 2.0.10.Final #14043

Closed
wants to merge 1 commit into from

Conversation

masahitojp
Copy link
Contributor

@masahitojp masahitojp commented Feb 23, 2021

Bump jboss-marshalling to 2.0.10.Final


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

  • Choose reviewer(s) and mention them in a comment (R: @username).
  • Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
  • 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.

Post-Commit Tests Status (on master branch)

Lang SDK Dataflow Flink Samza Spark Twister2
Go Build Status --- Build Status --- Build Status ---
Java Build Status Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status Build Status
Build Status
Build Status
Build Status
Python Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
--- Build Status ---
XLang Build Status Build Status Build Status --- Build Status ---

Pre-Commit Tests Status (on master branch)

--- Java Python Go Website Whitespace Typescript
Non-portable Build Status Build Status
Build Status
Build Status
Build Status
Build Status Build Status Build Status Build Status
Portable --- Build Status --- --- --- ---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

GitHub Actions Tests Status (on master branch)

Build python source distribution and wheels
Python tests
Java tests

See CI.md for more information about GitHub Actions CI.

@codecov
Copy link

codecov bot commented Feb 23, 2021

Codecov Report

Merging #14043 (6353519) into master (83bae26) will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #14043      +/-   ##
==========================================
- Coverage   82.98%   82.97%   -0.01%     
==========================================
  Files         469      469              
  Lines       58300    58300              
==========================================
- Hits        48381    48376       -5     
- Misses       9919     9924       +5     
Impacted Files Coverage Δ
...amples/snippets/transforms/aggregation/distinct.py
...ython/apache_beam/io/external/generate_sequence.py
...ild/srcs/sdks/python/apache_beam/utils/profiler.py
...s/python/apache_beam/io/gcp/bigquery_avro_tools.py
...rcs/sdks/python/apache_beam/typehints/typehints.py
...srcs/sdks/python/apache_beam/dataframe/__init__.py
...eam/runners/interactive/options/capture_control.py
...cs/sdks/python/apache_beam/typehints/decorators.py
...d/srcs/sdks/python/apache_beam/internal/pickler.py
...am/examples/complete/juliaset/juliaset/juliaset.py
... and 928 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 83bae26...6353519. Read the comment docs.

@masahitojp
Copy link
Contributor Author

Run Java_Examples_Dataflow PreCommit

@masahitojp
Copy link
Contributor Author

R: @iemejia
It's used in grpc and has passed the test so it seems safe to update

@pabloem pabloem requested a review from iemejia February 25, 2021 22:17
@iemejia
Copy link
Member

iemejia commented Mar 1, 2021

I am not sure how can we properly test this upgrade given that this is in the vendoring dependency, and fully testing it would probably require a release of the vendored dependency.

@masahitojp
Copy link
Contributor Author

@iemejia Thank you for your comment! I see.

I found suztomo-san doing this kind of PR right now. It may be better to wait for this to end.
#14028

If you know of any other people you should consult with, please let me know. 🙏

@iemejia
Copy link
Member

iemejia commented Mar 10, 2021

Since @suztomo seems to be working on the dependency upgrade I think we should close this one in favor of the major upgrade. Is the dependency upgraded in the new targetted vendored version of gRPC @suztomo ?

@suztomo
Copy link
Contributor

suztomo commented Mar 10, 2021

I'm now feeling this dependency is not needed for Beam's use of gRPC. It seems that the dependency was added in https://github.com/apache/beam/pull/10578/files to remove the errors from Linkage Checker. If Beam's using gRPC does not touch JBoss marshalling, we don't need the dependency here. (This means the Linkage Checker was showing false positive errors)

@masahitojp Do you see Beam (or Beam's use of gRPC) uses jboss-marshalling?

@aaltay
Copy link
Member

aaltay commented Mar 18, 2021

@masahitojp - Would you be able to look at the last question from @suztomo ?

@suztomo
Copy link
Contributor

suztomo commented Apr 1, 2021

Now jboss-marshalling has been removed from the vendored gRPC dependencies. I think you don't need this PR any more.

@aaltay aaltay closed this Apr 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants