Skip to content

Conversation

@armiol
Copy link
Contributor

@armiol armiol commented Jun 20, 2022

Prior to this changeset, there was no convenient way to set some "native" properties of the gRPC server, which is running underneath Spine's GrpcContainer.

In particular, it is often needed to set the maximum size for the inbound messages. And it was nearly not possible to achieve, other than using a test-only GrpcContainer.injectServer() method. There is also a number of other configuration endpoints provided by the gRPC's ServerBuilder, which could not be accessed.

In this PR, the gRPC's ServerBuilder API becomes exposed like this:

GrpcContainer.atPort(1654)
             // `server` is an instance of `io.grpc.ServerBuilder`.
             .withServer((server) -> server.maxInboundMessageSize(16_000_000))
             // ...
             .build();

So, rather than copying the ServerBuilder's API in GrpcContainer, the direct access to the builder is provided. That will allow to be always up-to-date with the configuration capabilities of the gRPC Server, rather than copying and chasing their API in the gRPC releases to come.

However, as of this PR, the GrpcContainer.Builder.withServer(...) method is marked as @Experimental. The reason is that the "native" ServerBuilder API allows to perform some destructive actions. Therefore, the decision to expose it as-is is not yet final.

The library version is set to 1.8.2. The release notes will follow.

@armiol armiol self-assigned this Jun 20, 2022
@armiol armiol requested a review from dmdashenkov June 20, 2022 10:19
@armiol
Copy link
Contributor Author

armiol commented Jun 20, 2022

@dmdashenkov PTAL.

Copy link
Contributor

@dmdashenkov dmdashenkov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@armiol, LGTM but for a single naming suggestion.

.build();
GrpcContainer container =
GrpcContainer.atPort(port)
.apply((server) -> server.addService(service))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's make the naming a bit more precise.

GrpcContainer.atPort(port)
             .withServer(s -> s.addService(service))
             .build();

Method apply(..) seems a bit too general. On another minor note, it's going to cause confusion on Kotlin and, probably, some extra syntax trickery to resolve the clash for the Kotlin compiler.

@armiol armiol merged commit f33d010 into 1.x-dev Jun 20, 2022
@armiol armiol deleted the 1.x-configure-grpc-server branch June 20, 2022 11:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants