Skip to content

Commit

Permalink
BufferDecoderGroupBuilder#add method w/out advertised (#2254)
Browse files Browse the repository at this point in the history
Motivation:
BufferDecoderGroupBuilder#add has an `advertised` parameter
which often is true and the impacts of it being false may not
be clear.

Modifications:
- Add BufferDecoderGroupBuilder#add with no `advertised` param
- Clarify javadoc on BufferDecoderGroupBuilder#add method for
  `advertised == false` implications.
  • Loading branch information
Scottmitch authored Jun 17, 2022
1 parent de5dca6 commit 89a92b7
Show file tree
Hide file tree
Showing 8 changed files with 42 additions and 23 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,11 @@ public interface BufferDecoderGroup {

/**
* Get the combined encoding to advertise. This is typically a combination of
* {@link BufferDecoder#encodingName()} contained in this group.
* {@link BufferDecoder#encodingName()} contained in this group. This value is commonly used in
* {@code Accept-Encoding} (or equivalent) metadata to advertise/communicate the supported algorithms.
* @return the combined encoding to advertise. This is typically a combination of
* {@link BufferDecoder#encodingName()} contained in this group.
* {@link BufferDecoder#encodingName()} contained in this group. This value is commonly used in
* {@code Accept-Encoding} (or equivalent) metadata to advertise/communicate the supported algorithms.
*/
@Nullable
CharSequence advertisedMessageEncoding();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,25 @@ public BufferDecoderGroupBuilder(int decodersSizeEstimate) {
/**
* Add a new {@link BufferDecoder} to the {@link BufferDecoderGroup} built by this builder.
* @param decoder The decoder to add.
* @param advertised {@code true} if the decoder should be included in
* {@link BufferDecoderGroup#advertisedMessageEncoding()}.
* @return {@code this}.
*/
public BufferDecoderGroupBuilder add(BufferDecoder decoder) {
return add(decoder, true);
}

/**
* Add a new {@link BufferDecoder} to the {@link BufferDecoderGroup} built by this builder.
* @param decoder The decoder to add.
* @param advertised
* <ul>
* <li>{@code true} - the decoder should be included in
* {@link BufferDecoderGroup#advertisedMessageEncoding()}</li>
* <li>{@code false} -the decoder is excluded from
* {@link BufferDecoderGroup#advertisedMessageEncoding()} and therefore won't be included in
* {@code Accept-Encoding} (or equivalent) metadata headers. In this case the peer won't be explicitly be told
* this decoder is supported. Commonly used to discourage usage of {@code identity} decoders in favor of other
* more preferred options, but still support it as a fallback if there are no common decoders.</li>
* </ul>
* @return {@code this}.
*/
public BufferDecoderGroupBuilder add(BufferDecoder decoder, boolean advertised) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,8 @@ public static void main(String... args) throws Exception {
// For the purposes of this example we disable GZip compression and use the
// server's second choice (deflate) to demonstrate that negotiation of compression algorithm is
// handled correctly.
// .add(NettyBufferEncoders.gzipDefault(), true)
.add(deflateDefault(), true)
// .add(NettyBufferEncoders.gzipDefault())
.add(deflateDefault())
.add(identityEncoder(), false).build()))) {
// This request is sent with the request being uncompressed. The response may
// be compressed because the ClientFactory will include the encodings we
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,8 @@ public static void main(String... args) throws Exception {
GrpcServers.forPort(8080)
.listenAndAwait(new Greeter.ServiceFactory.Builder()
.bufferDecoderGroup(new BufferDecoderGroupBuilder()
.add(gzipDefault(), true)
.add(deflateDefault(), true)
.add(gzipDefault())
.add(deflateDefault())
.add(identityEncoder(), false).build())
.bufferEncoders(asList(gzipDefault(), deflateDefault(), identityEncoder()))
.addService((GreeterService) (ctx, request) ->
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,8 @@ public static void main(String... args) throws Exception {
// For the purposes of this example we disable GZip compression and use the
// server's second choice (deflate) to demonstrate that negotiation of compression algorithm is
// handled correctly.
// .add(NettyBufferEncoders.gzipDefault(), true)
.add(deflateDefault(), true)
// .add(NettyBufferEncoders.gzipDefault())
.add(deflateDefault())
.add(identityEncoder(), false).build()))
.build()) {
// Make a request with an uncompressed payload.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@ public static void main(String... args) throws Exception {
.appendServiceFilter(new ContentEncodingHttpServiceFilter(
asList(gzipDefault(), deflateDefault(), identityEncoder()),
new BufferDecoderGroupBuilder()
.add(gzipDefault(), true)
.add(deflateDefault(), true)
.add(gzipDefault())
.add(deflateDefault())
.add(identityEncoder(), false).build()))
.listenAndAwait((ctx, request, responseFactory) -> {
String who = request.payloadBody(textSerializerUtf8());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1196,7 +1196,7 @@ private static BufferDecoderGroup serviceTalkDecompression(@Nullable final Strin
}
BufferDecoderGroupBuilder builder = new BufferDecoderGroupBuilder(2);
if (compression.contentEquals(NettyBufferEncoders.gzipDefault().encodingName())) {
builder.add(NettyBufferEncoders.gzipDefault(), true);
builder.add(NettyBufferEncoders.gzipDefault());
} else if (compression.contentEquals(Identity.identityEncoder().encodingName())) {
builder.add(Identity.identityEncoder(), false);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -121,17 +121,17 @@ static String payloadAsString(byte b) {

protected enum Decoders {
DEFAULT(EmptyBufferDecoderGroup.INSTANCE),
GZIP_ONLY(new BufferDecoderGroupBuilder().add(gzipDefault(), true).build()),
GZIP_ID(new BufferDecoderGroupBuilder().add(gzipDefault(), true).add(identityEncoder(), false).build()),
GZIP_DEFLATE_ID(new BufferDecoderGroupBuilder().add(gzipDefault(), true).add(deflateDefault(), true)
GZIP_ONLY(new BufferDecoderGroupBuilder().add(gzipDefault()).build()),
GZIP_ID(new BufferDecoderGroupBuilder().add(gzipDefault()).add(identityEncoder(), false).build()),
GZIP_DEFLATE_ID(new BufferDecoderGroupBuilder().add(gzipDefault()).add(deflateDefault())
.add(identityEncoder(), false).build()),
ID_ONLY(new BufferDecoderGroupBuilder().add(identityEncoder(), true).build()),
ID_GZIP(new BufferDecoderGroupBuilder().add(identityEncoder(), false).add(gzipDefault(), true).build()),
ID_DEFLATE(new BufferDecoderGroupBuilder().add(identityEncoder(), false).add(deflateDefault(), true).build()),
ID_DEFLATE_GZIP(new BufferDecoderGroupBuilder().add(identityEncoder(), false).add(deflateDefault(), true)
.add(gzipDefault(), true).build()),
DEFLATE_ONLY(new BufferDecoderGroupBuilder().add(deflateDefault(), true).build()),
DEFLATE_ID(new BufferDecoderGroupBuilder().add(deflateDefault(), true).add(identityEncoder(), false).build());
ID_ONLY(new BufferDecoderGroupBuilder().add(identityEncoder()).build()),
ID_GZIP(new BufferDecoderGroupBuilder().add(identityEncoder(), false).add(gzipDefault()).build()),
ID_DEFLATE(new BufferDecoderGroupBuilder().add(identityEncoder(), false).add(deflateDefault()).build()),
ID_DEFLATE_GZIP(new BufferDecoderGroupBuilder().add(identityEncoder(), false).add(deflateDefault())
.add(gzipDefault()).build()),
DEFLATE_ONLY(new BufferDecoderGroupBuilder().add(deflateDefault()).build()),
DEFLATE_ID(new BufferDecoderGroupBuilder().add(deflateDefault()).add(identityEncoder(), false).build());

final BufferDecoderGroup group;

Expand Down

0 comments on commit 89a92b7

Please sign in to comment.