Consolidation of HTTP codecs with gRPC #1198

tkountis · 2020-11-05T22:25:24Z

Motivation:

Consolidate HTTP codec work with gRPC codecs for a unified API and implementation.

Modifications:

Re-use HTTP codecs for the non streaming version of HTTP.
Adhere to the same changes applied for HTTP, to allow for codec ordering.

Result:

Unified API & implementation between HTTP. & gRPC.

tkountis · 2020-11-05T22:27:21Z

servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/GrpcMessageEncoding.java

- * API for <a href="https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md#message-encoding"> message
- * coding schemes</a>.
- */
-public interface GrpcMessageEncoding {


I am on the fence about removing this. I guess we could keep this around and use this interface for gRPC (delegating to the HTTP codec), allowing for logic separation if we need it in the future. @idelpivnitskiy thoughts?

Can you please clarify why you expect the need for gRPC-specific type?

I can't think of something, that's why I removed it.
I am just considering whether keeping it allows for some flexibility on doing changes to one without affecting both.

After thinking about it more, I'm agreed, it may be safer to have protocol-specific types that delegate the actual compression/decompression work to protocol-agnostic types.
We may have servicetalk-content-encoding-api module that will have ContentCodec class and identity, gzip, deflate implementations. Later we can create servicetalk-content-encoding-netty for netty-based implementations.

Then we may have HttpContentEncoding for http-api and GrpcMessageEncoding for grpc modules. Each will delegate to the necessary API of ContentCodec + have an additional method to set up required header value:

For HttpContentEncoding:

default HttpContentEncoding populateHeaders(final HttpHeader headers) { headers.add(CONTENT_ENCODING, name()); headers.add(VARY, CONTENT_ENCODING); return this; }

For GrpcMessageEncoding (should it extend HttpContentEncoding?):

@Override default GrpcMessageEncoding populateHeaders(final HttpHeader headers) { headers.set(GRPC_MESSAGE_ENCODING_KEY, name()); return this; }

WDYT?

servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/GrpcMessageEncodings.java

...rotobuf/src/main/java/io/servicetalk/grpc/protobuf/ProtoBufSerializationProviderBuilder.java

servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/GrpcMessageEncodings.java

idelpivnitskiy · 2020-11-06T04:21:33Z

servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/GrpcMessageEncoding.java

- * API for <a href="https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md#message-encoding"> message
- * coding schemes</a>.
- */
-public interface GrpcMessageEncoding {


Can you please clarify why you expect the need for gRPC-specific type?

servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/GrpcUtils.java

tkountis · 2020-11-23T14:26:45Z

test this please

idelpivnitskiy

Overall looks great!
Mostly minor comments:

idelpivnitskiy · 2020-11-24T08:06:21Z

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

+
+        @Override
+        public long skip(long n) {
+            int skipped = min(buffer.readableBytes(), (int) min(Integer.MAX_VALUE, n));


buffer.readableBytes() is int. Can we simplify this to (int) min(buffer.readableBytes(), n)?

We should also account for negative n. The original skip method from the superclass does:

if (n <= 0) { return 0; }

I added the negative guard, but the cast to integer might not be the right way.
Currently the max allowed input is INT_MAX, if I relax that and cast the input, that could mean that an arbitrary input of (long 4294967297 -> int 1) can be used to skip 1 bytes rather than all available bytes.
Unexpected behavior.

You don't need to cast the input, min(buffer.readableBytes(), n) will cast readableBytes to long. Because readableBytes is int, it can not be more than Integer.MAX_VALUE. Then the long result of the min function can safely be cast back to int: (int) min(buffer.readableBytes(), n).

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

idelpivnitskiy · 2020-11-24T08:16:22Z

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

     * @param length the total length available for reading
     * @param allocator the {@link BufferAllocator} to use for allocating auxiliary buffers or the returned buffer
     * @return {@link Buffer} the result buffer with the content encoded
     */
-    Buffer encode(Buffer src, int offset, int length, BufferAllocator allocator);
+    Buffer encode(Buffer src, int length, BufferAllocator allocator);


Discussed offline and confirmed that we actually need the variant with length for gRPC because the received Buffer may contain more data that we want to encode/decode. In this case, the offset, limit is more standard API approach. Can you please reverth the offset? Sorry for back and forth.

It is more standard API, but in our case it seems (to me) more confusing.
Buffer has the reader/writer indexes, which these methods mutate. So we typically mutate the readerIndex after consuming the buffer, to current_readerIndex + limit. If we support offset, that can be different than the readerIndex, (before or after), what happens to the readerIndex if the offset + limit < current_readerIndex? Do we move it back to adhere to the contract or keep it unchanged?
I think since the Buffer API allows easy positioning of the readerIndex, we should allow the caller to rely on that for offsetting.

WDYT?

Good point. Let's discuss offline what should be the default behavior of all codecs and deserializers. We are currently inconsistent. If we will decide to move indexes everywhere, then having only length here is reasonable. If we decide not to move indexes, then offset may be important.

servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/DefaultGrpcServiceContext.java

servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/GrpcUtils.java

...rotobuf/src/main/java/io/servicetalk/grpc/protobuf/ProtoBufSerializationProviderBuilder.java

tkountis · 2020-11-25T18:14:33Z

test this please

idelpivnitskiy

A few minor comments and good to go. We can discuss the indexes move and offset param for aggregated API in a follow-up.

idelpivnitskiy · 2020-11-30T21:12:56Z

servicetalk-buffer-api/src/main/java/io/servicetalk/buffer/api/BufferInputStream.java

@@ -1,5 +1,5 @@
 /*
- * Copyright © 2018 Apple Inc. and the ServiceTalk project authors
+ * Copyright © 2020 Apple Inc. and the ServiceTalk project authors


We should not override the year, we can append or change a region. Examples:
2018 -> 2018, 2020
2019 -> 2019-2020
2018-2019 -> 2018-2020

idelpivnitskiy · 2020-12-01T01:10:13Z

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

        final Buffer dst = allocator.newBuffer(chunkSize);
        DeflaterOutputStream output = null;
        try {
            output = newDeflaterOutputStream(Buffer.asOutputStream(dst));

            if (src.hasArray()) {
                output.write(src.array(), src.arrayOffset() + src.readerIndex(), length);
+                src.readerIndex(src.readerIndex() + length);


Good catch to make both paths consistent 🎖️

idelpivnitskiy · 2020-12-01T01:12:26Z

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

@@ -246,6 +262,7 @@ public void onNext(@Nullable final Buffer src) {
                    // Not enough data to decompress, ask for more
                    subscription.request(1);
                } catch (Exception e) {
+                    LOGGER.error("Error while decoding with " + name(), e);


Here and in all other places: consider using logger API to build a String instead of concatenating manually:
LOGGER.error("Error while decoding with {}", name(), e);

idelpivnitskiy · 2020-12-01T01:48:32Z

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

+
+        @Override
+        public long skip(long n) {
+            int skipped = min(buffer.readableBytes(), (int) min(Integer.MAX_VALUE, n));


You don't need to cast the input, min(buffer.readableBytes(), n) will cast readableBytes to long. Because readableBytes is int, it can not be more than Integer.MAX_VALUE. Then the long result of the min function can safely be cast back to int: (int) min(buffer.readableBytes(), n).

idelpivnitskiy · 2020-12-01T01:51:51Z

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

@@ -67,6 +72,7 @@ default Buffer decode(Buffer src, BufferAllocator allocator) {

    /**
     * Take a {@link Buffer} and decode its contents resulting in a {@link Buffer} with the decoded content.
+     * This call increases the {@code readerIndex} of the {@code src} with the number of bytes read {@code length}.


Let's defer adding these details in javadoc?

Since we concluded on moving indexes, I kept these changes too.
I think this is now consistent with other Buffer APIs.

idelpivnitskiy · 2020-12-01T02:03:24Z

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

     * @param length the total length available for reading
     * @param allocator the {@link BufferAllocator} to use for allocating auxiliary buffers or the returned buffer
     * @return {@link Buffer} the result buffer with the content encoded
     */
-    Buffer encode(Buffer src, int offset, int length, BufferAllocator allocator);
+    Buffer encode(Buffer src, int length, BufferAllocator allocator);


Good point. Let's discuss offline what should be the default behavior of all codecs and deserializers. We are currently inconsistent. If we will decide to move indexes everywhere, then having only length here is reasonable. If we decide not to move indexes, then offset may be important.

idelpivnitskiy · 2020-12-01T02:04:12Z

servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/GrpcSerializationProvider.java

+     * List of supported {@link ContentCodec}s for this {@link GrpcSerializationProvider}.
+     * Content codings will be used to encoded and decode gRPC messages according to configuration of client and server.
+     *
+     * @return list of supported {@link ContentCodec}s for this {@link GrpcSerializationProvider}


This is not fixed, reminder :)

tkountis requested a review from idelpivnitskiy November 5, 2020 22:25

tkountis self-assigned this Nov 5, 2020

tkountis commented Nov 5, 2020

View reviewed changes

tkountis changed the title ~~Grpc encoding concolidation~~ Consolidation of HTTP codecs with gRPC Nov 5, 2020

idelpivnitskiy mentioned this pull request Nov 5, 2020

Introduce HTTP content encoding H1 & H2 #1174

Merged

idelpivnitskiy reviewed Nov 6, 2020

View reviewed changes

tkountis force-pushed the http-compression branch from 8a16823 to 39ec439 Compare November 17, 2020 16:08

Base automatically changed from http-compression to main November 19, 2020 10:18

tkountis force-pushed the grpc-encoding-concolidation branch 3 times, most recently from 6701c4a to 28b12dd Compare November 23, 2020 13:04

tkountis requested a review from idelpivnitskiy November 23, 2020 13:11

Consolidate Http codecs with gRPC

c50b348

tkountis force-pushed the grpc-encoding-concolidation branch from 28b12dd to c50b348 Compare November 23, 2020 15:41

idelpivnitskiy requested changes Nov 24, 2020

View reviewed changes

tkountis requested a review from idelpivnitskiy November 25, 2020 00:07

tkountis force-pushed the grpc-encoding-concolidation branch from 257b545 to c49ceb4 Compare November 25, 2020 16:06

Fix comments & other improvements

ad3c67e

tkountis force-pushed the grpc-encoding-concolidation branch from c49ceb4 to ad3c67e Compare November 25, 2020 17:13

idelpivnitskiy approved these changes Dec 1, 2020

View reviewed changes

Comments

94081be

tkountis merged commit bb2a77c into main Dec 3, 2020

tkountis deleted the grpc-encoding-concolidation branch December 3, 2020 23:41

tkountis restored the grpc-encoding-concolidation branch December 3, 2020 23:42

tkountis deleted the grpc-encoding-concolidation branch December 4, 2020 00:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consolidation of HTTP codecs with gRPC #1198

Consolidation of HTTP codecs with gRPC #1198

tkountis commented Nov 5, 2020 •

edited

Loading

tkountis Nov 5, 2020

idelpivnitskiy Nov 6, 2020

tkountis Nov 6, 2020

idelpivnitskiy Nov 6, 2020 •

edited

Loading

idelpivnitskiy Nov 6, 2020

tkountis commented Nov 23, 2020

idelpivnitskiy left a comment

idelpivnitskiy Nov 24, 2020

tkountis Nov 24, 2020

idelpivnitskiy Dec 1, 2020

tkountis Dec 1, 2020

idelpivnitskiy Nov 24, 2020

tkountis Nov 24, 2020 •

edited

Loading

idelpivnitskiy Dec 1, 2020

tkountis commented Nov 25, 2020

idelpivnitskiy left a comment

idelpivnitskiy Nov 30, 2020

idelpivnitskiy Dec 1, 2020

idelpivnitskiy Dec 1, 2020

idelpivnitskiy Dec 1, 2020

idelpivnitskiy Dec 1, 2020

tkountis Dec 3, 2020

idelpivnitskiy Dec 1, 2020

idelpivnitskiy Dec 1, 2020

Consolidation of HTTP codecs with gRPC #1198

Consolidation of HTTP codecs with gRPC #1198

Conversation

tkountis commented Nov 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

idelpivnitskiy Nov 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkountis commented Nov 23, 2020

idelpivnitskiy 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

tkountis Nov 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkountis commented Nov 25, 2020

idelpivnitskiy 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

tkountis commented Nov 5, 2020 •

edited

Loading

idelpivnitskiy Nov 6, 2020 •

edited

Loading

tkountis Nov 24, 2020 •

edited

Loading