Extract grpc protocol to separate artifact and add AbstractUnaryGrpcService. #1727
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
grpc/src/main/java/com/linecorp/armeria/common/grpc/protocol/AbstractUnaryGrpcService.java
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## master #1727 +/- ##
============================================
+ Coverage 72.9% 72.94% +0.03%
- Complexity 8140 8151 +11
============================================
Files 734 736 +2
Lines 32419 32465 +46
Branches 3986 3989 +3
============================================
+ Hits 23636 23680 +44
- Misses 6751 6755 +4
+ Partials 2032 2030 -2
Continue to review full report at Codecov.
|
hyangtack
approved these changes
Apr 22, 2019
grpc/src/main/java/com/linecorp/armeria/common/grpc/protocol/AbstractUnaryGrpcService.java
Outdated
Show resolved
Hide resolved
|
wonder if you need to rebase this in order to get the gradle + JITPACK goodness or not.. |
|
at any rate I can just use snapshot to test :P |
|
Jitpack is set up by the depending project (e.g, zipkin), don't think we'd need to do anything here for you to try it |
|
@anuraaga sorry.. didn't tag you on this reference line/gradle-scripts#55 |
5 tasks
grpc/src/test/java/com/linecorp/armeria/common/grpc/protocol/AbstractUnaryGrpcServiceTest.java
Show resolved
Hide resolved
|
looks like still a dep on grpc's utilities, used for the following header |
|
testing by inlining "armeria.grpc.ThrowableProto-bin" vs using code to generate it |
|
right now I am building with a small patch. I still have the exception above, just trying to figure out what to do about it. diff --git a/grpc/src/main/java/com/linecorp/armeria/common/grpc/GrpcHeaderNames.java b/grpc/src/main/java/com/linecorp/armeria/common/grpc/GrpcHeaderNames.java
index b14f5442..0c0bb342 100644
--- a/grpc/src/main/java/com/linecorp/armeria/common/grpc/GrpcHeaderNames.java
+++ b/grpc/src/main/java/com/linecorp/armeria/common/grpc/GrpcHeaderNames.java
@@ -17,8 +17,6 @@
package com.linecorp.armeria.common.grpc;
import com.linecorp.armeria.common.HttpHeaderNames;
-
-import io.grpc.protobuf.ProtoUtils;
import io.netty.util.AsciiString;
/**
@@ -49,7 +47,7 @@ public final class GrpcHeaderNames {
* {@code "armeria.grpc.ThrowableProto-bin"}.
*/
public static final AsciiString ARMERIA_GRPC_THROWABLEPROTO_BIN =
- HttpHeaderNames.of(ProtoUtils.keyForProto(ThrowableProto.getDefaultInstance()).name());
+ HttpHeaderNames.of("armeria.grpc.ThrowableProto-bin");
private GrpcHeaderNames() {}
} |
|
this also crashes the server.. looking into it now. |
|
Ah I think the doc service stuff will get triggered as long as armeria-grpc is on the classpath, using service loader. This would be a reason to extract a separate artifact for |
|
I've a try/catch in progress.. just testing now --- a/core/src/main/java/com/linecorp/armeria/server/docs/DocService.java
+++ b/core/src/main/java/com/linecorp/armeria/server/docs/DocService.java
@@ -32,6 +32,7 @@ import java.util.Set;
import java.util.function.BiFunction;
import java.util.stream.Collectors;
+import java.util.stream.Stream;
import javax.annotation.Nullable;
import com.fasterxml.jackson.annotation.JsonInclude.Include;
@@ -178,8 +179,14 @@ public class DocService extends AbstractCompositeService<HttpRequest, HttpRespon
private static ServiceSpecification addDocStrings(ServiceSpecification spec, List<ServiceConfig> services) {
final Map<String, String> docStrings =
plugins.stream()
- .flatMap(plugin -> plugin.loadDocStrings(findSupportedServices(plugin, services))
- .entrySet().stream())
+ .flatMap(plugin -> {
+ try { // Guard when optional classpath dependencies are unmet
+ return plugin.loadDocStrings(findSupportedServices(plugin, services))
+ .entrySet().stream();
+ } catch (NoClassDefFoundError e) {
+ return Stream.empty();
+ }
+ })
.collect(toImmutableMap(Entry::getKey, Entry::getValue, (a, b) -> a));
return new ServiceSpecification( |
|
ok with the two diffs I posted the server starts. the response is different, which I'm looking into now. Seems like a missing content type header. before $ curl -X POST localhost:9411/zipkin.proto3.SpanService/Report -H'Content-Type: application/grpc' -v
* Trying ::1...
* TCP_NODELAY set
* Connected to localhost (::1) port 9411 (#0)
> POST /zipkin.proto3.SpanService/Report HTTP/1.1
> Host: localhost:9411
> User-Agent: curl/7.54.0
> Accept: */*
> Content-Type: application/grpc
>
< HTTP/1.1 200 OK
< content-type: application/grpc+proto
< grpc-status: 13
< grpc-message: Half-closed without a request
< content-length: 0
<
* Connection #0 to host localhost left intactnow (plus the two patches) $ curl -X POST localhost:9411/zipkin.proto3.SpanService/Report -H'Content-Type: application/grpc' -v
* Trying ::1...
* TCP_NODELAY set
* Connected to localhost (::1) port 9411 (#0)
> POST /zipkin.proto3.SpanService/Report HTTP/1.1
> Host: localhost:9411
> User-Agent: curl/7.54.0
> Accept: */*
> Content-Type: application/grpc
>
< HTTP/1.1 200 OK
< grpc-encoding: identity
< transfer-encoding: chunked
<
* Connection #0 to host localhost left intact |
grpc/src/main/java/com/linecorp/armeria/common/grpc/protocol/AbstractUnaryGrpcService.java
Outdated
Show resolved
Hide resolved
|
ps hold a tick as I think the path I'm looking at isn't actually required.. |
|
here's my entire code now.. same thing with the difference in curl result (also with grpc client fail). Not sure if there's a smoking gun here. service SpanService {
rpc Report(zipkin.proto3.ListOfSpans) returns (google.protobuf.Empty) {}
}@ConditionalOnProperty(name = "zipkin.collector.grpc.enabled") // disabled by default
final class ZipkinGrpcCollector {
@Bean ArmeriaServerConfigurator grpcCollectorConfigurator(StorageComponent storage,
CollectorSampler sampler, CollectorMetrics metrics) {
CollectorMetrics grpcMetrics = metrics.forTransport("grpc");
Collector collector = Collector.newBuilder(getClass())
.storage(storage)
.sampler(sampler)
.metrics(grpcMetrics)
.build();
return sb ->
sb.service("/zipkin.proto3.SpanService/Report", new SpanService(collector, grpcMetrics));
}
static final class SpanService extends AbstractUnaryGrpcService {
final Collector collector;
final CollectorMetrics metrics;
SpanService(Collector collector, CollectorMetrics metrics) {
this.collector = collector;
this.metrics = metrics;
}
@Override protected CompletableFuture<byte[]> handleMessage(byte[] bytes) {
metrics.incrementMessages();
CompletableFutureCallback result = new CompletableFutureCallback();
collector.acceptSpans(bytes, SpanBytesDecoder.PROTO3, result);
return result;
}
}
static final class CompletableFutureCallback extends CompletableFuture<byte[]>
implements Callback<Void> {
@Override public void onSuccess(Void value) {
complete(new byte[0] /* empty result */);
}
@Override public void onError(Throwable t) {
completeExceptionally(t);
}
}
} |
|
it seems the thing grpc's client dislikes most is the missing content-type in the response |
|
Yeah will add that in, that was not intended. Will also update the test to use upstream client instead of armeria client to catch that, since it's more important to test that anyways. Thanks for checking out the code! |
|
actually there's a bit tougher issue trying to exclude protobuf (which reduces the jar cost of this integration to near nothing). serviceloader pukes on .hasNext() |
|
Ready for another look. Extracted a module, which solves doc service issue and ensures no more accidental usage of gRPC dependencies. And fixed content type header, upstream client went from not working to working in a unit test. @adriancole think you should be good. I merged master so jitpack might work too |
|
trying now thanks! |
|
the anticipation! cc @bsideup |
|
dunno why it adds in |
|
ignore the jitpack related comments.. will follow-up on these elsewhere |
|
shipit! this works https://github.com/apache/incubator-zipkin/pull/2328/files |
|
I think we can merge this and do a follow-up on |
|
Yeah I'm good but just to clarify, what do you mean by follow up? Do you mean the name of the artifact? |
|
@anuraaga That's because I didn't know you finished the separation already. That was fast. 👍 |
|
Could you update the PR description and commit message finally so I can merge this PR? |
anuraaga
changed the title
|
Updated description :) |
|
@hyangtack Could you take another look since it was updated since your last review? |
|
Thanks, @anuraaga and @adriancole ! |
Merged
trustin
pushed a commit
that referenced
this pull request
May 2, 2019
Fixes #1703 This is the client version of #1727. Advanced users can use UnaryGrpcClient to issue gRPC-framed requests to a gRPC server given they handle serialization / deserialization of messages themselves, either by depending on protobuf (without needing to depend on grpc) or reimplementing the logic themselves. As this is an advanced API, I think it's good to keep it small since it won't be as well tested as normal APIs. I've only exposed the most flexible APIs without providing any extra helpers as a result.
fmguerreiro
pushed a commit
to fmguerreiro/armeria
that referenced
this pull request
Sep 19, 2020
…ervice. (line#1727) For line#1703 Tried extracting a package within the armeria-grpc artifact that only contains wire format logic, but it is fragile due to accidental dependencies and classpath magic like service loaders, which we use. So now we extract to a separate artifact entirely. `AbstractUnaryGrpcService` allows defining unary gRPC services that are provided the direct bytes of a single gRPC message and return the bytes of a single gRPC message, and the base class takes cafe of the gRPC wire protocol. As this is an advanced use case, it doesn't attempt to implement everything in gRPC but should hopefully provide enough for most armeria-grpc users that are concerned about having dependencies on stub libraries.
fmguerreiro
pushed a commit
to fmguerreiro/armeria
that referenced
this pull request
Sep 19, 2020
Fixes line#1703 This is the client version of line#1727. Advanced users can use UnaryGrpcClient to issue gRPC-framed requests to a gRPC server given they handle serialization / deserialization of messages themselves, either by depending on protobuf (without needing to depend on grpc) or reimplementing the logic themselves. As this is an advanced API, I think it's good to keep it small since it won't be as well tested as normal APIs. I've only exposed the most flexible APIs without providing any extra helpers as a result.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For #1703
Tried extracting a package within the armeria-grpc artifact that only contains wire format logic, but it is fragile due to accidental dependencies and classpath magic like service loaders, which we use. So now we extract to a separate artifact entirely.
AbstractUnaryGrpcServiceallows defining unary gRPC services that are provided the direct bytes of a single gRPC message and return the bytes of a single gRPC message, and the base class takes cafe of the gRPC wire protocol. As this is an advanced use case, it doesn't attempt to implement everything in gRPC but should hopefully provide enough for most armeria-grpc users that are concerned about having dependencies on stub libraries.@adriancole can you check whether this works for you or provide any suggestions?