-
Notifications
You must be signed in to change notification settings - Fork 68
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
base: main
Are you sure you want to change the base?
fix: S2A - skip S2A on platforms where netty-tcnative is not available #3802
Conversation
@lqiu96 , would you be able to review this when you get a chance? Thank you! |
if (System.getProperty("os.name").contains("Windows") || | ||
(System.getProperty("os.name").contains("OS X") | ||
&& System.getProperty("os.arch").contains("x86_64"))) { | ||
return false; | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this 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
Done. |
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.