Skip to content

chore: convert to static reflect-config.json using client generator #1841

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
Merged
burkedavison merged 21 commits into from
Jul 17, 2023
Merged

chore: convert to static reflect-config.json using client generator #1841

burkedavison merged 21 commits into from
Jul 17, 2023

Conversation

@burkedavison
Copy link
Member

@burkedavison burkedavison commented Jul 10, 2023 •
edited
Loading

A reflect-config.json is now generated as part of the client.

  • The previous ProtobufMessageFeature that uses build-time logic to dynamically register classes for reflection has been removed from the gax-grpc native configuration.
  • RunReachabilityHandlersConcurrently is no longer being disabled in the gax-grpc native configuration.
  • Showcase native testing verifies the configuration works.
  • Golden integration tests and golden showcase tests now include reflect-config.json in their file set.

Showcase native-test binary size on Mac w/GraalVM 22.3.0 Java 17 CE:

  • With ProtobufMessageFeature: 102.7MB
  • With reflect-config.json: 89.8MB

@burkedavison burkedavison added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jul 10, 2023
@product-auto-label product-auto-label bot added the size: xl Pull request size is extra large. label Jul 10, 2023
@burkedavison
Copy link
Member Author

burkedavison commented Jul 12, 2023

Latest commit intended to break the golden tests, and did.
Good.
Fixing now.

@burkedavison burkedavison removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jul 12, 2023
@burkedavison burkedavison marked this pull request as ready for review July 12, 2023 21:14
@burkedavison burkedavison requested a review from a team as a code owner July 12, 2023 21:14
@burkedavison burkedavison requested a review from meltsufin July 13, 2023 00:27
@burkedavison burkedavison added kokoro:force-run Add this label to force Kokoro to re-run the tests. owlbot:run Add this label to trigger the Owlbot post processor. labels Jul 13, 2023
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Jul 13, 2023
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 13, 2023
@burkedavison burkedavison added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 13, 2023
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 13, 2023
burkedavison
burkedavison commented Jul 13, 2023
View reviewed changes
...rpc/src/main/resources/META-INF/native-image/com.google.api/gax-grpc/native-image.properties
Comment on lines 13 to 16
--features=com.google.api.gax.grpc.nativeimage.ProtobufMessageFeature,\
com.google.api.gax.grpc.nativeimage.GrpcNettyFeature \
-H:-RunReachabilityHandlersConcurrently
-H:-RunReachabilityHandlersConcurrently
Copy link
Member Author

@burkedavison burkedavison Jul 13, 2023

Choose a reason for hiding this comment

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

ProtobufMessageFeature and -H:-RunReachabilityHandlersConcurrently have now been reintroduced.

Implementation plan:

  1. Merge and release with this PR's changes.
  2. Wait for Owlbot to use the updated gapic-generator-java, and generate the reflect-config.json for each client libary.
  3. In the next release cycle: Remove ProtobufMessageFeature and -H:-RunReachabilityHandlersConcurrently, perform downstream testing on the client libraries that will then have local reflect-configs.

meltsufin
meltsufin approved these changes Jul 13, 2023
View reviewed changes
Copy link
Member

@meltsufin meltsufin left a comment

Choose a reason for hiding this comment

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

Please add an issue to track removal of the feature in the next release cycle, if you haven't already.

@burkedavison burkedavison mentioned this pull request Jul 13, 2023
Closed
@burkedavison
Copy link
Member Author

burkedavison commented Jul 13, 2023

#1844 created and CC'd release managers @JoeWang1127 and @blakeli0 for visibility.

blakeli0
blakeli0 approved these changes Jul 13, 2023
View reviewed changes
mpeddada1
mpeddada1 approved these changes Jul 13, 2023
View reviewed changes
Copy link
Contributor

@mpeddada1 mpeddada1 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This looks great.
Pending one minor comment to add the TODO (discussed offline) about the structure of the META-INF/native/image directory. Not an issue rn as we are only expecting one reflect-config.json per library but in the future we may need to enhance this logic a bit to follow convention (https://www.graalvm.org/22.1/reference-manual/native-image/BuildConfiguration/#embedding-a-configuration-file) so that the configs don't get overridden if we have have two files that have the same file path.

@burkedavison
Copy link
Member Author

burkedavison commented Jul 13, 2023

@mpeddada1 : This looks great. Pending one minor comment...

Done.

blakeli0
blakeli0 reviewed Jul 14, 2023
View reviewed changes
Copy link
Contributor

@blakeli0 blakeli0 left a comment

Choose a reason for hiding this comment

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

LGTM.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jul 14, 2023

[gapic-generator-java-root] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 3 Code Smells

93.7% 93.7% Coverage
0.0% 0.0% Duplication

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jul 14, 2023

[java_showcase_integration_tests] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jul 14, 2023

[java_showcase_unit_tests] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@burkedavison burkedavison merged commit 194097a into main Jul 17, 2023
@burkedavison burkedavison deleted the showcase-reflect-generation branch July 17, 2023 15:58
This was referenced Jul 19, 2023
Merged
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@blakeli0 blakeli0 blakeli0 approved these changes

@meltsufin meltsufin meltsufin approved these changes

@mpeddada1 mpeddada1 mpeddada1 approved these changes

Assignees

No one assigned

Labels

size: xl Pull request size is extra large.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@burkedavison @blakeli0 @meltsufin @mpeddada1 @yoshi-kokoro