Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[1.x] Allow to configure the underlying gRPC server #1454

Merged
merged 9 commits into from
Jun 20, 2022

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.

2 participants