Skip to content
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

Support streaming services for JSON transcoding #5832

Open
jrhee17 opened this issue Jul 26, 2024 · 10 comments
Open

Support streaming services for JSON transcoding #5832

jrhee17 opened this issue Jul 26, 2024 · 10 comments

Comments

@jrhee17
Copy link
Contributor

jrhee17 commented Jul 26, 2024

Currently, Armeria's HttpJsonTranscodingService doesn't support streaming messages and throws an exception.

checkArgument(methodDefinition.getMethodDescriptor().getType() == MethodType.UNARY,
"Only unary methods can be configured with an HTTP/JSON endpoint: " +
"method=%s, httpRule=%s",
methodDefinition.getMethodDescriptor().getFullMethodName(), httpRule);

Recently, we saw an issue where some users with streaming services in their proto files weren't able to use transcoding at all since the above exception was thrown. #5828 #5829

Although a workaround was added to log a warning instead of throwing an exception, this also probably isn't ideal since users will still see warning logs for which they can't (and shouldn't) take action.

Moreover, it seems like grpc-gateway supports streaming as seen in the following issue where ndjson is used: grpc-ecosystem/grpc-gateway#581

In an effort to 1) remove this discrepancy with upstream and 2) remove unnecessary warning logs, I suggest that we also support streaming messages for HttpJsonTranscodingService.

@jrhee17
Copy link
Contributor Author

jrhee17 commented Jul 26, 2024

Note: At least to follow-up on #5829, we may want to consider returning a 405 Method Not Allowed when a request to a streaming service is made until this issue is fully addressed.

@dipeshsingh253
Copy link

dipeshsingh253 commented Oct 1, 2024

Hi @jrhee17 , I would like to contribute to this issue. Can you please assign it to me.?

@jrhee17
Copy link
Contributor Author

jrhee17 commented Oct 2, 2024

Sure, assigned

@dipeshsingh253
Copy link

Hi @jrhee17 ,

I followed the docs and tried to build Armeria locally using ./gradlew --parallel build but faced some failed test cases.

image

@dipeshsingh253
Copy link

dipeshsingh253 commented Oct 4, 2024

Hi @jrhee17, @ikhoon , I have successfully set up the dev environment in locally.

I went through the HttpJsonTranscodingService.java file. Currently, we are just logging in case of nonunary.

Currently, two things I can see are mentioned, but I am not sure how to approach them yet. Can you please explain in brief or redirected me to some resource which might help me.

In an effort to 1) remove this discrepancy with upstream and 2) remove unnecessary warning logs, I suggest that we also support streaming messages for HttpJsonTranscodingService.

@jrhee17
Copy link
Contributor Author

jrhee17 commented Oct 7, 2024

Currently once HttpJsonTranscodingService receives a request, the request is aggregated in memory until the end of stream is received.

https://github.com/line/armeria/blob/main/grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.java#L640

In order to support streaming client streaming rpc, we will likely need to

  1. divide the request according to a delimiter (newline)
  2. convert each json into a protobuf
  3. delegate the newly created request body to the logic processing gRPC requests (ref)

In order to support server streaming rpc, we likely need to do something similar to the response body. i.e. modify to convert each protobuf message to a json, and then send each newline delimited json.

private static Function<AggregatedHttpResponse, AggregatedHttpResponse> generateResponseConverter(

Lastly, using reactive streams may not be performant so we may want to keep the current logic for unary and add a separate code path for streaming requests/responses.
We can probably check the content-type in the headers to determine whether a request is meant to be unary or streaming.

@jrhee17
Copy link
Contributor Author

jrhee17 commented Oct 7, 2024

I missed this comment: #5832 (comment)

You may specify ./gradlew -Pretry=true -PfailOnPassedAfterRetry=false to retry flaky tests automatically

@ikhoon
Copy link
Contributor

ikhoon commented Oct 7, 2024

I would like to contribute to this issue. Can you please assign it to me.?

First, thanks for your interest. However, I don't think this issue is good for the first contributor. This issue requires some experience with Armeria internals and gRPC specifications to implement.

What do you think of finding other issues that are simpler and do not need to modify many files? It might be good to warm up and get familiar with the Armeria code base while working on an easy issue.

@dipeshsingh253
Copy link

What do you think of finding other issues that are simpler and do not need to modify many files? It might be good to warm up and get familiar with the Armeria code base while working on an easy issue.

How can I find easy or good to-start issues in armeria??

@ikhoon
Copy link
Contributor

ikhoon commented Oct 7, 2024

We've added a good first issue label to issues that we thought had low difficulty and its scope is limited. https://github.com/line/armeria/issues?q=sort:updated-desc+is:issue+is:open+label:%22good+first+issue%22

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants