Skip to content

fix: S2A - skip S2A on platforms where netty-tcnative is not available #3802

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

rmehta19
Copy link
Contributor

S2A has a runtime dependency on netty-tcnative.

netty-tcnative is looking to drop support for windows and macos intel based builds. To avoid runtime errors for users of the Cloud SDK, disable S2A if they are running on these platforms.

@product-auto-label product-auto-label bot added the size: s Pull request size is small. label May 19, 2025
@rmehta19 rmehta19 marked this pull request as draft May 19, 2025 22:16
@rmehta19 rmehta19 marked this pull request as ready for review May 28, 2025 16:58
@rmehta19
Copy link
Contributor Author

@lqiu96 , would you be able to review this when you get a chance? Thank you!

Comment on lines 316 to 320
if (System.getProperty("os.name").contains("Windows") ||
(System.getProperty("os.name").contains("OS X")
&& System.getProperty("os.arch").contains("x86_64"))) {
return false;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

qq, this is a dependency brought in by grpc-s2a, right?

Just wanted to confirm as I don't think this is brought in by Gax.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I also feel this is some low level implementation details that Gax does not need to be aware of, can this logic be handled in grpc-java instead of Gax?

Copy link
Contributor Author

@rmehta19 rmehta19 Jun 10, 2025

Choose a reason for hiding this comment

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

Hi @lqiu96 and @blakeli0 apologies for the delayed response.

qq, this is a dependency brought in by grpc-s2a, right?Just wanted to confirm as I don't think this is brought in by Gax.

Yes, this is a dependency of grpc-s2a. This dependency is dropping support on the specified platforms so we don't want to try and use S2A when we are on those platforms.

can this logic be handled in grpc-java instead of Gax

This is a good point, technically, I believe the answer is yes, we could just return null / throw an error if you try and use S2A on these platforms. However, there are a few more things I need to confirm, I will follow up with you both and grpc-java internally after I investigate and confirm a few things, and then we can proceed with this PR accordingly.

lqiu96
lqiu96 previously approved these changes Jun 2, 2025
Copy link
Contributor

@lqiu96 lqiu96 left a comment

Choose a reason for hiding this comment

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

Can you also update the title to use conventional commits

@rmehta19 rmehta19 changed the title Skip S2A on platforms where netty-tcnative is not available fix: S2A - skip S2A on platforms where netty-tcnative is not available Jun 10, 2025
@rmehta19
Copy link
Contributor Author

Can you also update the title to use conventional commits

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: s Pull request size is small.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants