Skip to content

compiler: generate blocking v2 unary calls that throw StatusException #12126

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 4 commits into
base: master
Choose a base branch
from

Conversation

shivaspeaks
Copy link
Member

@shivaspeaks shivaspeaks commented Jun 3, 2025

Note on API-breaking change:

This change modifies the blocking unary call behavior in v2 stubs to throw a checked StatusException instead of an unchecked StatusRuntimeException.

Existing code using v1 stubs will continue to behave as before — no changes are required for v1.
For users not ready to adopt the new behavior in v2, you may continue using code generated with v1 stub style until you’re ready to migrate.

Fixes #11937

@@ -182,6 +182,23 @@ public static <ReqT, RespT> RespT blockingUnaryCall(
}
}

/**
* Executes a unary call and blocks on the response, converting StatusRuntimeException to
* StatusException for consistency with other blocking call types.
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove "for consistency with other blocking call types."

@kannanjgithub
Copy link
Contributor

You have included the generated stubs in the commit for interop-testing but not for android-interop-testing which causes Android build to fail.

@shivaspeaks
Copy link
Member Author

shivaspeaks commented Jun 4, 2025

I see.
So, interop-testing files were generated automatically after clean build but we do keep the flag -PskipAdroid=true to skip android related builds.

@kannanjgithub
Copy link
Contributor

You can install Android sdk following these steps and then do a build with Android build enabled. If you run into OOM during the build then we can look into upgrading the cloudtop or using a custom Kokoro VM and work on that VM.

@shivaspeaks shivaspeaks marked this pull request as ready for review June 4, 2025 14:02
@shivaspeaks shivaspeaks requested a review from ejona86 June 4, 2025 14:02
@ejona86
Copy link
Member

ejona86 commented Jun 4, 2025

If you run into OOM during the build then we can look into upgrading the cloudtop or using a custom Kokoro VM and work on that VM.

If you get an OOM, it normally isn't really the machine you're on. You generally just need to tell Gradle to use more memory. I think I put org.gradle.jvmargs=-Xmx2048m in my gradle.properties for that.

Copy link
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

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

Make some note in the commit/PR description how users should handle the API-breaking nature of the change (i.e., v1 will worth the same before/after this change, and they can stay on the old generated code for a little bit).

@@ -182,6 +182,23 @@ public static <ReqT, RespT> RespT blockingUnaryCall(
}
}

/**
* Executes a unary call and blocks on the response, converting StatusRuntimeException
Copy link
Member

Choose a reason for hiding this comment

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

Converting the exceptions is an implementation detail. You can say it uses a checked exception, but don't say it is "converting"

@shivaspeaks
Copy link
Member Author

If you get an OOM, it normally isn't really the machine you're on. You generally just need to tell Gradle to use more memory. I think I put org.gradle.jvmargs=-Xmx2048m in my gradle.properties for that.

Yes right, I have kept org.gradle.jvmargs=-Xmx4096m 👀

@shivaspeaks
Copy link
Member Author

I don't know how to resolve this Android failure. Android build in local works just fine, there's no reason to fail. @kannanjgithub

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.

blocking v2 client stub inconsistent about StatusRuntimeException verus StatusException
3 participants