New Serializer APIs, consolidation of ContentCodec, and gRPC MethodDescriptor #1673

Scottmitch · 2021-07-16T00:41:26Z

Motivation:
Supporting a custom transport for gRPC currently requires
reflection and special knowledge of types for serialization. The
generated code has information related to types and serialization that
can be exposed to the runtime which also allows supporting custom
transports.

This task requires exposing serializer APIs into the public
API. Our existing serializer APIs were heavily influced by streaming use
cases and JSON serialization which can serialize any type and generally
doesn't have type restrictions. This lead to a general
SerializationProvider API which is impossible to constrain types due to
the generics being at the method level, which doesn't account for
implementing serialization for type constrained approached (protobuf
MessageLite). Also the serialization APIs couple scalar and streaming
use cases which in practice are more commonly decoupled into scalar
(individual objects) and streaming (which applies some framing around
individual objects). These concepts are coupled which leads to confusing
specializations (deserializeAggregatedSingle, deserializeAggregated)
which are easy to misuse and lead to invalid serialization in practice
(streaming {form url, string} encoding are not properly framed and may
not be deserialized correctly). The existing Serializer APIs also do not
promote composability in that if you can serialize one object you should
be able to re-use that serialization for a stream of objects with some
frameing applied on top (required for gRPC).

Modifications:

Introduce a new servicetalk-serializer-api package with new
Serializer APIs.
Introduce a new servicetalk-serializer-utils package with common
utilities for serialization such as streaming framing (fixed and
varint length prefixed)
Introduce BufferEncoder APIs in servicetalk-encoding-api for
compression and decompression. This also comes with
NettyBufferEncoders and NettyCompression to create Netty backed
implementations.
Introduce HttpStreamingSerializer and new HttpSerializer APIs in
servicetalk-http-api, and modify all request/response APIs to use the
new serializer APIs. Introduce new methods to deserialize and process
trailers.
Introduce HttpSerializers in servicetalk-http-api for implementations
of the new APIs.
Introduce JacksonSerializerCache in servicetalk-http-api as the new
entry point to create/cache serializers for JSON.
Introduce ProtobufSerializerCache in servicetalk-data-protobuf as the
new entry point to create/cache serializers for protobuf.
Introduce MethodDescriptor API into servicetalk-grpc-api and update
code generation to use this new API.
Update all examples avoid usage of deprecated APIs and leverage new
APIs.
Deprecate the following APIs and related methods in favor of the new
types descrbied above:
- SerializationProvider, JacksonSerializationProvider,
  ProtobufSerializationProvider, GrpcSerializationProvider,
  HttpSerializationProvider
- ContentCodec, CodecDecodingException, CodecEncodingException,
  ContentCodings
- GrpcMetadata#path(), code generated types that extend
  DefaultGrpcClientMetadata

Result:
gRPC API supports MethodDescriptor, which uses new Serializer APIs, and
the entire code base has been updated to use the new APIs consistently.

Scottmitch · 2021-07-16T13:11:55Z

test failure attributed to #1579

Scottmitch · 2021-07-16T15:00:20Z

build failure attributed to #1663

Scottmitch · 2021-07-16T17:15:20Z

build failure attributed to #1663

bondolo

70/304 files so far…

servicetalk-http-api/src/main/java/io/servicetalk/http/api/HttpHeaderValues.java

servicetalk-http-api/src/main/java/io/servicetalk/http/api/HttpDeserializer2.java

...izer-utils/src/main/java/io/servicetalk/serializer/utils/FixedLengthStreamingSerializer.java

...cetalk-serializer-api/src/main/java/io/servicetalk/serializer/api/StreamingDeserializer.java

...-serializer-utils/src/main/java/io/servicetalk/serializer/utils/StringCharsetSerializer.java

...alk-serializer-utils/src/main/java/io/servicetalk/serializer/utils/StringUtf8Serializer.java

Scottmitch · 2021-07-17T02:33:17Z

another #1579

Scottmitch · 2021-07-17T05:03:49Z

another #1663

Scottmitch · 2021-07-19T17:06:47Z

another #1663

Scottmitch · 2021-07-20T22:12:56Z

another #1579 and #1663

Scottmitch · 2021-07-21T05:54:14Z

#1507 and #1663

Scottmitch · 2021-07-21T20:37:20Z

build failure attributed to #1579

bondolo

approve for examples portion

bondolo · 2021-07-22T21:45:20Z

...p/debugging/src/main/java/io/servicetalk/examples/http/debugging/DebuggingExampleServer.java

@@ -26,8 +26,7 @@
 import java.util.function.BooleanSupplier;

 import static io.servicetalk.concurrent.api.Single.succeeded;
-import static io.servicetalk.http.api.HttpSerializationProviders.textDeserializer;
-import static io.servicetalk.http.api.HttpSerializationProviders.textSerializer;
+import static io.servicetalk.http.api.HttpSerializers.textSerializerUtf8;


It seems odd to see textSerializerUtf8 in sites that are deserialization. Perhaps naming it textSerializationUtf8 would be less suggestive that the usage is serializing rather than deserializing.

.../java/io/servicetalk/examples/http/helloworld/async/streaming/HelloWorldStreamingClient.java

.../serialization/src/main/java/io/servicetalk/examples/http/serialization/SerializerUtils.java

tkountis · 2021-07-30T22:31:49Z

...icetalk-http-api/src/main/java/io/servicetalk/http/api/ContentEncodingHttpServiceFilter.java

+ * Append this filter before others that are expected to to see compressed content for this request/response, and after
+ * other filters that expect to see/manipulate the original payload.
+ */
+public final class ContentEncodingHttpServiceFilter


nit: Coding as noun has both the meaning of encoding or decoding. I think the name before made more sense. The comment is worth considering across the whole encoding/decoding APIs (didn't want to duplicate it).

related/duplicate of #1673 (comment) ... lets discuss there if necessary.

tkountis

1st round.
General comments:

that I didn't want to repeat, it's probably worth sticking to ser/deser naming convention to all implementations.
new APIs are certainly better and I love the effort with varint streaming support.

...icetalk-http-api/src/main/java/io/servicetalk/http/api/ContentEncodingHttpServiceFilter.java

tkountis · 2021-07-30T22:44:34Z

...icetalk-http-api/src/main/java/io/servicetalk/http/api/ContentEncodingHttpServiceFilter.java

+    }
+
+    @Nullable
+    static <T> T matchAndRemoveEncoding(final List<T> supportedEncoders,


anyway we can extract parts of this to remove duplication between this and HeaderUtils#negotiateAcceptedEncodingRaw

I tried sharing code between these two methods but they are doing different jobs and I couldn't find a good way. I considered at least moving them into the same HeaderUtils file, to keep similar code close but the matchAndRemoveEncoding can be kept package private if it stays here.

matchAndRemoveEncoding -> needs to remove the encoding header that is matched at preserve ordering (bcz the body has been decoded and the header no longer applies). first header must supported values or else we can't decode and its an error.
negotiateAcceptedEncodingRaw -> finds a matching encoding, so it can be applied (no removal of headers). any match is acceptable.

servicetalk-http-api/src/main/java/io/servicetalk/http/api/HttpRequestMetaData.java

tkountis · 2021-07-30T23:13:52Z

...serializer-utils/src/main/java/io/servicetalk/serializer/utils/AbstractStringSerializer.java

+
+import static io.servicetalk.utils.internal.CharsetUtils.standardCharsets;
+
+abstract class AbstractStringSerializer implements SerializerDeserializer<String> {


Serializer/Deserializer in name? same for subclass

A shorthand in the form of SerDe is frequently used in the community.

The strategy I used was to be explicit with the interface names (SerializerDeserializer, BufferEncoderDecoder) but just use the "serializer" variant for concrete method/type names that fill both roles for brevity (StringSerializer, NettyBufferEncoders#bufferEncoder(..), HttpSerializers#textSerializerUtf8()). In practice I'm not sure lengthening the names of concrete types/methods, or using abbreviations (SerDe), will improve understandability/readability.

...zer-utils/src/main/java/io/servicetalk/serializer/utils/VarIntLengthStreamingSerializer.java

tkountis · 2021-07-30T23:24:16Z

servicetalk-data-jackson/src/main/java/io/servicetalk/data/jackson/JacksonSerializer.java

+ * Serializes and deserializes to/from JSON via jackson.
+ * @param <T> The type of objects to serialize.
+ */
+final class JacksonSerializer<T> implements SerializerDeserializer<T> {


Consider Serialiazer/Deserializer in name

duplicate of #1673 (comment) ... lets discuss there if necessary.

...etalk-data-jackson/src/main/java/io/servicetalk/data/jackson/JacksonStreamingSerializer.java

chemicL · 2021-08-02T17:09:19Z

...lk-http-api/src/main/java/io/servicetalk/http/api/BlockingStreamingHttpMessageBodyUtils.java

+        };
+    }
+
+    private static final class BlockingStreamingHttpMessageBodyAdapter<I> implements BlockingIterator<Buffer> {


Should access to this class' state be made thread-safe? BlockingIterator comes from the io.servicetalk.concurrent package. I'd assume it can be used in a concurrent context. The parent class has no mention of it requiring single-thread access.

No this isn't necessary. BlockingIterator is in concurrent package because it is how we convert to/from java.util.Iterator related types. This type just adds timeouts at the call site and makes iterable Closable (which will cancel the underlying publisher if it isn't fully consumed).

Scottmitch · 2021-08-02T21:03:33Z

build failure attributed to #1579

idelpivnitskiy

I will submit my review in chunks:

...alk-serializer-api/src/main/java/io/servicetalk/serializer/api/StreamingSerializerUtils.java

...serialization-api/src/main/java/io/servicetalk/serialization/api/SerializationException.java

...talk-serializer-utils/src/main/java/io/servicetalk/serializer/utils/ByteArraySerializer.java

...izer-utils/src/main/java/io/servicetalk/serializer/utils/FixedLengthStreamingSerializer.java

...serializer-utils/src/main/java/io/servicetalk/serializer/utils/AbstractStringSerializer.java

...cetalk-encoding-api/src/main/java/io/servicetalk/encoding/api/BufferDecoderGroupBuilder.java

servicetalk-encoding-api/src/main/java/io/servicetalk/encoding/api/BufferEncodingException.java

servicetalk-encoding-api/src/main/java/io/servicetalk/encoding/api/CodecDecodingException.java

...alk-encoding-api/src/main/java/io/servicetalk/encoding/api/IdentityBufferEncoderDecoder.java

...-encoding-netty/src/main/java/io/servicetalk/encoding/netty/DefaultBufferEncoderDecoder.java

Scottmitch · 2021-08-05T02:19:35Z

build failure attributed to #1579

Scottmitch · 2021-08-05T03:55:51Z

another #1579

idelpivnitskiy

So far the approach looks good! Lots of improvements.
I still need to look at grpc. Everything else is reviewed. But don't be blocked by that.

servicetalk-data-jackson/src/main/java/io/servicetalk/data/jackson/JacksonSerializerCache.java

...data-jackson/src/test/java/io/servicetalk/data/jackson/JacksonSerializationProviderTest.java

...src/main/java/io/servicetalk/data/jackson/jersey/SerializationExceptionMapperDeprecated.java

servicetalk-data-jackson/src/main/java/io/servicetalk/data/jackson/JacksonSerializerCache.java

servicetalk-data-protobuf/src/main/java/io/servicetalk/data/protobuf/ProtobufSerializer.java

servicetalk-http-api/src/main/java/io/servicetalk/http/api/HeaderUtils.java

...tp-api/src/main/java/io/servicetalk/http/api/DefaultHttpStreamingSerializerDeserializer.java

servicetalk-http-api/src/main/java/io/servicetalk/http/api/HttpSerializers.java

servicetalk-http-api/src/main/java/io/servicetalk/http/api/StreamingHttpPayloadHolder.java

…scriptor Motivation: Supporting a custom transport for gRPC currently requires reflection and special knowledge of types for serialization. The generated code has information related to types and serialization that can be exposed to the runtime which also allows supporting custom transports. This task requires exposing serializer APIs into the public API. Our existing serializer APIs were heavily influced by streaming use cases and JSON serialization which can serialize any type and generally doesn't have type restrictions. This lead to a general SerializationProvider API which is impossible to constrain types due to the generics being at the method level, which doesn't account for implementing serialization for type constrained approached (protobuf MessageLite). Also the serialization APIs couple scalar and streaming use cases which in practice are more commonly decoupled into scalar (individual objects) and streaming (which applies some framing around individual objects). These concepts are coupled which leads to confusing specializations (deserializeAggregatedSingle, deserializeAggregated) which are easy to misuse and lead to invalid serialization in practice (streaming {form url, string} encoding are not properly framed and may not be deserialized correctly). The existing Serializer APIs also do not promote composability in that if you can serialize one object you should be able to re-use that serialization for a stream of objects with some frameing applied on top (required for gRPC). Modifications: - Introduce a new servicetalk-serializer-api package with new Serializer APIs. - Introduce a new servicetalk-serializer-utils package with common utilities for serialization such as streaming framing (fixed and varint length prefixed) - Introduce BufferEncoder APIs in servicetalk-encoding-api for compression and decompression. This also comes with NettyBufferEncoders and NettyCompression to create Netty backed implementations. - Introduce HttpStreamingSerializer and new HttpSerializer APIs in servicetalk-http-api, and modify all request/response APIs to use the new serializer APIs. Introduce new methods to deserialize and process trailers. - Introduce HttpSerializers in servicetalk-http-api for implementations of the new APIs. - Introduce JacksonSerializerCache in servicetalk-http-api as the new entry point to create/cache serializers for JSON. - Introduce ProtobufSerializerCache in servicetalk-data-protobuf as the new entry point to create/cache serializers for protobuf. - Introduce MethodDescriptor API into servicetalk-grpc-api and update code generation to use this new API. - Update all examples avoid usage of deprecated APIs and leverage new APIs. - Deprecate the following APIs and related methods in favor of the new types descrbied above: - SerializationProvider, JacksonSerializationProvider, ProtobufSerializationProvider, GrpcSerializationProvider, HttpSerializationProvider - ContentCodec, CodecDecodingException, CodecEncodingException, ContentCodings - GrpcMetadata#path(), code generated types that extend DefaultGrpcClientMetadata Result: gRPC API supports MethodDescriptor, which uses new Serializer APIs, and the entire code base has been updated to use the new APIs consistently. make control flow in ConnectionCloseHeaderHandlingTest and GracefulConnectionClosureHandlingTest more robust

…elimitedTo

Scottmitch · 2021-08-07T01:31:16Z

servicetalk-http-api/src/main/java/io/servicetalk/http/api/BlockingStreamingHttpRequest.java

+     * Get the {@link HttpMessageBodyIterable} for this request.
+     * @return the {@link HttpMessageBodyIterable} for this request.
+     */
+    HttpMessageBodyIterable<Buffer> messageBody();


continuation of #1673 (comment)

@idelpivnitskiy @chemicL - note that I removed BlockingStreamingHttpMessageBody originally introduced in this PR in favor of HttpMessageBodyIterable (happy to discuss/iterate). The motivation is the functional APIs such as TrailersTransformer doesn't lend themselves to the sequential programming style of traditional blocking APIs. I meant to deprecate the transform methods on the blocking request/response APIs but forgot (will push this change). The BlockingIterable types are still cumbersome to create but I think that can be improved over time (more utilities) and also we have a task to investigate removal of BlockingIterable (in favor of ClosableIterable).

Scottmitch · 2021-08-07T17:28:41Z

build failure attributed to #1294

Scottmitch · 2021-08-07T18:16:43Z

build failure attributed to #1507

Scottmitch requested review from bondolo, tkountis and idelpivnitskiy July 16, 2021 00:41

Scottmitch force-pushed the grpc_method_descriptor branch from a3dcaab to 79481dc Compare July 16, 2021 13:37

Scottmitch requested a review from chemicL July 16, 2021 13:44

Scottmitch force-pushed the grpc_method_descriptor branch from a31c3a5 to 313c05d Compare July 16, 2021 16:48

bondolo assigned Scottmitch Jul 16, 2021

bondolo added API PR with API changes (New or Deprecated) breaking-change PR with breaking changes, removed APIs or other binary incompatibilities. labels Jul 16, 2021

bondolo reviewed Jul 16, 2021

View reviewed changes

Scottmitch force-pushed the grpc_method_descriptor branch 2 times, most recently from 52cfb1f to f20e3c6 Compare July 17, 2021 00:03

Scottmitch force-pushed the grpc_method_descriptor branch from 6dfba1d to 1b4c5e9 Compare July 20, 2021 22:01

Scottmitch force-pushed the grpc_method_descriptor branch 2 times, most recently from a164ae6 to b623507 Compare July 21, 2021 20:26

bondolo approved these changes Jul 22, 2021

View reviewed changes

tkountis reviewed Jul 30, 2021

View reviewed changes

chemicL reviewed Aug 2, 2021

View reviewed changes

idelpivnitskiy reviewed Aug 3, 2021

View reviewed changes

idelpivnitskiy reviewed Aug 6, 2021

View reviewed changes

Scottmitch added 17 commits August 6, 2021 17:48

add tests for varint compatability with parseDelimitedFrom and writeD…

fb024d7

…elimitedTo

review comments, reduce StringSerializer public API surface

a39b1c1

add missing comments for StreamingSerializer

54c66d2

use StandardCharsets

5d122dc

clarify comments

1d126d4

add comment to timeout example, improve javadoc

b19bb09

catch SerializationException in NettyHttpServer

35bd388

update NettyHttpServer to return 415 for SerializationException

3b4955e

review comments

9be8f9e

review comments

da84640

fix spotbugs issue

276fc5c

more review comments. lift conditional out of loop for VarInt serializer

6bded76

update tests 4 -> Integer.BYTES

a38d942

more comments, improve HttpSerializers.bytesStreamingSerializer

e2200c6

fix javadoc for bytesStreamingSerializer

7b55b24

idel review comments round 2

b6761a7

Scottmitch force-pushed the grpc_method_descriptor branch from 9d8543d to b6761a7 Compare August 7, 2021 00:48

Scottmitch added 2 commits August 6, 2021 17:58

more code sharing

845387b

rename Cache -> Factory

507e5cf

Scottmitch commented Aug 7, 2021

View reviewed changes

more cleanup

16db64e

Scottmitch merged commit 4ce1aa6 into apple:main Aug 7, 2021

Scottmitch deleted the grpc_method_descriptor branch August 7, 2021 18:27

idelpivnitskiy mentioned this pull request Oct 29, 2021

Adjust parent types for exceptions #1924

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Serializer APIs, consolidation of ContentCodec, and gRPC MethodDescriptor #1673

New Serializer APIs, consolidation of ContentCodec, and gRPC MethodDescriptor #1673

Scottmitch commented Jul 16, 2021

Scottmitch commented Jul 16, 2021

Scottmitch commented Jul 16, 2021

Scottmitch commented Jul 16, 2021

bondolo left a comment

Scottmitch commented Jul 17, 2021

Scottmitch commented Jul 17, 2021

Scottmitch commented Jul 19, 2021

Scottmitch commented Jul 20, 2021

Scottmitch commented Jul 21, 2021

Scottmitch commented Jul 21, 2021

bondolo left a comment

bondolo Jul 22, 2021

tkountis Jul 30, 2021

Scottmitch Aug 4, 2021 •

edited

Loading

tkountis left a comment

tkountis Jul 30, 2021

Scottmitch Aug 2, 2021

tkountis Jul 30, 2021

chemicL Aug 2, 2021

Scottmitch Aug 2, 2021

tkountis Jul 30, 2021

Scottmitch Aug 4, 2021

chemicL Aug 2, 2021

Scottmitch Aug 2, 2021

Scottmitch commented Aug 2, 2021

idelpivnitskiy left a comment

Scottmitch commented Aug 5, 2021

Scottmitch commented Aug 5, 2021

idelpivnitskiy left a comment

Scottmitch Aug 7, 2021 •

edited

Loading

Scottmitch commented Aug 7, 2021

Scottmitch commented Aug 7, 2021


		import static io.servicetalk.utils.internal.CharsetUtils.standardCharsets;

		abstract class AbstractStringSerializer implements SerializerDeserializer<String> {

New Serializer APIs, consolidation of ContentCodec, and gRPC MethodDescriptor #1673

New Serializer APIs, consolidation of ContentCodec, and gRPC MethodDescriptor #1673

Conversation

Scottmitch commented Jul 16, 2021

Scottmitch commented Jul 16, 2021

Scottmitch commented Jul 16, 2021

Scottmitch commented Jul 16, 2021

bondolo left a comment

Choose a reason for hiding this comment

Scottmitch commented Jul 17, 2021

Scottmitch commented Jul 17, 2021

Scottmitch commented Jul 19, 2021

Scottmitch commented Jul 20, 2021

Scottmitch commented Jul 21, 2021

Scottmitch commented Jul 21, 2021

bondolo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Scottmitch Aug 4, 2021 • edited Loading

Choose a reason for hiding this comment

tkountis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Scottmitch commented Aug 2, 2021

idelpivnitskiy left a comment

Choose a reason for hiding this comment

Scottmitch commented Aug 5, 2021

Scottmitch commented Aug 5, 2021

idelpivnitskiy left a comment

Choose a reason for hiding this comment

Scottmitch Aug 7, 2021 • edited Loading

Choose a reason for hiding this comment

Scottmitch commented Aug 7, 2021

Scottmitch commented Aug 7, 2021

Scottmitch Aug 4, 2021 •

edited

Loading

Scottmitch Aug 7, 2021 •

edited

Loading