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

Allow fine-tuning of gRPC server underneath of GrpcContainer #1510

Merged
merged 4 commits into from
Jun 2, 2023

Conversation

armiol
Copy link
Contributor

@armiol armiol commented May 31, 2023

This changeset is a port of #1454 to master.

Disclaimer

Also, this is just a first of such "porting" PRs. To keep things going, we are going to use the current state of core-java:master, rather than wait until the whole machinery (ProtoData et al) unwinds in its full blossom. Therefore, config, dependencies and stuff will NOT be updated in scope of "porting".

What's Changed

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.

@armiol armiol self-assigned this May 31, 2023
@codecov
Copy link

codecov bot commented May 31, 2023

Codecov Report

Merging #1510 (ec5c160) into master (936f719) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #1510      +/-   ##
============================================
+ Coverage     89.68%   89.70%   +0.01%     
- Complexity     4902     4904       +2     
============================================
  Files           631      631              
  Lines         15305    15312       +7     
  Branches        883      884       +1     
============================================
+ Hits          13726    13735       +9     
+ Misses         1273     1271       -2     
  Partials        306      306              

Copy link
Contributor

@alexander-yevsyukov alexander-yevsyukov left a comment

Choose a reason for hiding this comment

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

LGTM, which one minor suggestion.

@DisplayName("configure underlying gRPC server")
void configureUnderlyingGrpcServer() {
var port = 1654;
var service = CommandService
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we have .newBuilder() on the previous line, please?

Probably, we need to have a test fixture which would read something like CommandSerice.noOp() or CommandService.servingNoCommands().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have refrained from creating a fully-fledged fixture set, as we only have such cases thrice in two different tests. But I have created a new Given... on this matter.

@armiol armiol added this pull request to the merge queue Jun 2, 2023
Merged via the queue into master with commit 8388726 Jun 2, 2023
@armiol armiol deleted the configure-grpc-server branch June 2, 2023 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants