Skip to content

Conversation

@scwhittle
Copy link
Contributor

@scwhittle scwhittle commented Feb 21, 2024

Remove some uses of ClassLoadingStrategy.Default.INJECTION in favor of
ByteBuddyUtils.getClassLoadingStrategy which prevents issues with newer java versions.

See #25578 for more details

Related to #30358
There are more changes required in sdks/java/extensions/protobuf/src/main/java/org/apache/beam/sdk/extensions/protobuf/ProtoByteBuddyUtils.java but due to difficulty testing they will be done separately.


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)

Build python source distribution and wheels
Python tests
Java tests
Go tests

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

@scwhittle
Copy link
Contributor Author

R: @Abacn

@github-actions
Copy link
Contributor

Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control

@scwhittle scwhittle requested a review from Abacn February 26, 2024 09:55
@scwhittle scwhittle closed this Feb 26, 2024
@scwhittle scwhittle reopened this Feb 26, 2024
.visit(new AsmVisitorWrapper.ForDeclaredMethods().writerFlags(ClassWriter.COMPUTE_FRAMES))
.make()
.load(ReflectHelpers.findClassLoader(), ClassLoadingStrategy.Default.INJECTION)
.load(ReflectHelpers.findClassLoader(), getClassLoadingStrategy(protoClass))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

while the change of all other files follow the same pattern, the use case here looks slightly different. Would you mind providing some hint to the changes in ProroByteBuddyUtil ?

Or if there is anything not sure, we can leave this file untouched. I just recall the last time of this effort (#23210) broke some test and had some back and forth. I was wondering if any remaining usage Default.INJECTION not removed in #25578 was due to that it is nontrivial

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the class used was the one used for package injection in the helper method.
But since I'm not sure how to best test this and it wasn't directly causing issues for me I'm removing from this PR.

@github-actions github-actions bot removed the protobuf label Feb 26, 2024
@scwhittle scwhittle changed the title Remove remaining uses of ClassLoadingStrategy.Default.INJECTION Remove some uses of ClassLoadingStrategy.Default.INJECTION Feb 26, 2024
Copy link
Contributor

@Abacn Abacn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@scwhittle scwhittle merged commit 6406cfe into apache:master Feb 26, 2024
@scwhittle scwhittle deleted the rm_default_reflect branch February 26, 2024 20:34
@scwhittle scwhittle restored the rm_default_reflect branch April 2, 2024 07:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants