Untangling `GrpcServerBuilder` from `HttpServerBuilder` #1861

chemicL · 2021-10-04T18:01:04Z

Motivation:

The methods in GrpcServerBuilder are often only delegating to the
underlying HTTP transport builder. It will be beneficial if the API can
be simplified and have only gRPC specific settings. The first step is to
deprecate the methods which can be called directly on the underlying
HttpServerBuilder and to introduce a method for configuring that
builder.

Modifications:

Introduce a GrpcServerBuilder.HttpInitializer interface and a
corresponding GrpcServerBuilder#initialize method which accepts the
initializer for the underlying HttpServerBuilder instance,
Deprecate methods which have only served the purpose of delegating to
the underlying HTTP transport builder,
Refactor tests to use the new approach to showcase it works.

Result:

The API of GrpcServerBuilder is prepared for removal of the deprecated
methods in 0.42 release and the API is more specific, while maintaining
the same level of configurability.

Motivation: The methods in `GrpcServerBuilder` are often only delegating to the underlying HTTP transport builder. It will be beneficial if the API can be simplified and have only gRPC specific settings. The first step is to deprecate the methods which can be called directly on the underlying `HttpServerBuilder` and to introduce a method for configuring that builder. Modifications: - Introduce a `GrpcServerBuilder.HttpInitializer` interface and a corresponding `GrpcServerBuilder#initialize` method which accepts the initializer for the underlying `HttpServerBuilder` instance, - Deprecate methods which have only served the purpose of delegating to the underlying HTTP transport builder, - Refactor tests to use the new approach to showcase it works. Result: The API of `GrpcServerBuilder` is prepared for removal of the deprecated methods in 0.42 release and the API is more specific, while maintaining the same level of configurability.

tkountis

The deprecations will be removed for 0.42 correct?

tkountis · 2021-10-05T10:18:58Z

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

    private boolean appendedCatchAllFilter;

+    public abstract GrpcServerBuilder initializer(HttpInitializer initializer);


Does this affect the order of applied configurations? In other words, when we are in this transient state with the deprecations, if users call appendHttpServiceFilter directly (deprecated methods) and they also start using the initializer where they also might append filters, is the order of the filters consistent to what it would have been if that new API wasn't there? If thats the case, can we prevent bad configuration, e.g. allowing exclusively one way or the other.

Great observation! To answer – the order would not be preserved as the initializer is applied at build time, while the deprecated methods have an immediate effect on the underlying builder. We could add documentation, but that would not prevent users from possible issues. I will try to implement a mutually exclusive mechanism, bear with me.

Ok, I found where this comes from :)
We should not be extra caution in this scenario. Users either migrate all appendHttpServiceFilter methods or nothing. If they use a mixed approach - this is their problem, because we recommend migration, not mixing. We can clarify it in javadoc, but no need to add extra guards in the code. Neither multi-address nor partitioned client builders have similar guards.

That's one way of looking at it. It is not always the case that users can migrate everything at the same time - say they receive a preconfigured builder from their own library and append their own configurations. In this case it's probably better to actually fail instead of being forced to debug production issues which are not straightforward to understand. I was happy with @tkountis suggestion so I implemented the validation. But I agree, a consistent approach would be to not validate the use as the other builders didn't check the state. Also, an exception is perhaps too aggressive as it might happen at runtime in a situation that is not dramatic for the user and can go past automated tests where the logic is not validated. So I decided to remove the logic. But I believe it's a matter of opinion that ServiceTalk takes -> we could as well transition into a more safe-to-use/prevent-users-from-shooting-themselves-in-the-foot approach.

chemicL · 2021-10-05T10:53:11Z

The deprecations will be removed for 0.42 correct?

Yes, that is the intention to remove the deprecated methods.

tkountis

LGTM 🚢

idelpivnitskiy · 2021-10-05T15:57:44Z

servicetalk-grpc-netty/src/main/java/io/servicetalk/grpc/netty/DefaultGrpcServerBuilder.java

+            // been used to build a client.
+            if (initializer != null) {
+                if (underlyingBuilderCalledDirectly) {
+                    throw new IllegalStateException("Builder configured with " + HttpInitializer.class.getName() +


Why this is a problem to mix approaches? Top level methods delegate to httpServerBuilder, initializer also invokes methods on the same httpServerBuilder. Order should not matter for most of the settings except "appendFilter", which is fine.

I replied in the other thread as a general comment. Regarding order of operations -> it's not just appending filters that can be a problem.
Configuring anything else at the same time can be a surprise as the actual implementation controls the order of initializers. Consider the following code:

GrpcServers.forAddress(localAddress(0)) .initializeHttp(builder -> builder.sslConfig(trustedServerConfig())) .sslConfig(untrustedServerConfig()) .listenAndAwait(serviceFactory());

The actual SSL config will be trustedServerConfig() now and would also have been before my recent change. However the user might as well expect that the order of calls should determine the actual outcome and expect untrustedServerConfig() to be actually applied.

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

idelpivnitskiy · 2021-10-05T18:11:16Z

@bondolo this is not a breaking change, we expect to cherry-pick this into 0.41

…enamed initializer to initializeHttp

idelpivnitskiy

LGTM after the last comments addressed.
Nice change! Thanks @tkountis for a proposal ❤️

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

servicetalk-grpc-netty/src/main/java/io/servicetalk/grpc/netty/DefaultGrpcServerBuilder.java

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

idelpivnitskiy

Discovered when I reviewed the client-side:

servicetalk-grpc-netty/src/main/java/io/servicetalk/grpc/netty/DefaultGrpcServerBuilder.java

Motivation: The methods in `GrpcServerBuilder` are often only delegating to the underlying HTTP transport builder. It will be beneficial if the API can be simplified and have only gRPC specific settings. The first step is to deprecate the methods which can be called directly on the underlying `HttpServerBuilder` and to introduce a method for configuring that builder. Modifications: - Introduce a `GrpcServerBuilder.HttpInitializer` interface and a corresponding `GrpcServerBuilder#initialize` method which accepts the initializer for the underlying `HttpServerBuilder` instance, - Deprecate methods which have only served the purpose of delegating to the underlying HTTP transport builder, - Added default implementations for deprecated methods, - Made the builder reconfigurable after first build, - Clarified javadoc for lifecycleObserver overriding by initializer, - Refactor tests to use the new approach to showcase it works. Result: The API of `GrpcServerBuilder` is prepared for removal of the deprecated methods in 0.42 release and the API is more specific, while maintaining the same level of configurability.

chemicL · 2021-10-13T09:35:33Z

Cherry-picked for 0.41: f8aaa8d

chemicL requested review from bondolo, tkountis, idelpivnitskiy and Scottmitch October 4, 2021 18:01

tkountis reviewed Oct 5, 2021

View reviewed changes

tkountis assigned chemicL Oct 5, 2021

Protection against use of deprecated methods together with initializer

9d2e853

tkountis approved these changes Oct 5, 2021

View reviewed changes

Renamed private variable

4bd3921

idelpivnitskiy requested changes Oct 5, 2021

View reviewed changes

bondolo added API PR with API changes (New or Deprecated) breaking-change PR with breaking changes, removed APIs or other binary incompatibilities. labels Oct 5, 2021

idelpivnitskiy removed the breaking-change PR with breaking changes, removed APIs or other binary incompatibilities. label Oct 5, 2021

Removed guard against mixing initializer with deprecated method and r…

944a9cb

…enamed initializer to initializeHttp

chemicL requested a review from idelpivnitskiy October 6, 2021 15:15

idelpivnitskiy reviewed Oct 6, 2021

View reviewed changes

servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/GrpcServerBuilder.java Outdated Show resolved Hide resolved

idelpivnitskiy requested changes Oct 6, 2021

View reviewed changes

servicetalk-grpc-netty/src/main/java/io/servicetalk/grpc/netty/DefaultGrpcServerBuilder.java Outdated Show resolved Hide resolved

Dariusz Jędrzejczyk added 4 commits October 7, 2021 10:25

Added default implementations for deprecated methods

c432f1c

Made the builder reconfigurable after first build

b8fb579

Clarified javadoc for lifecycleObserver overriding by initializer

83b600d

Added javadoc for the append method

f18fed7

chemicL merged commit e892ff9 into apple:main Oct 11, 2021

idelpivnitskiy mentioned this pull request Oct 12, 2021

gRPC catch-all filters should implement HttpExecutionStrategyInfluencer #1884

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Untangling `GrpcServerBuilder` from `HttpServerBuilder` #1861

Untangling `GrpcServerBuilder` from `HttpServerBuilder` #1861

chemicL commented Oct 4, 2021

tkountis left a comment

tkountis Oct 5, 2021

chemicL Oct 5, 2021

idelpivnitskiy Oct 5, 2021 •

edited

Loading

chemicL Oct 7, 2021

chemicL commented Oct 5, 2021

tkountis left a comment

idelpivnitskiy Oct 5, 2021

chemicL Oct 7, 2021 •

edited

Loading

idelpivnitskiy commented Oct 5, 2021 •

edited

Loading

idelpivnitskiy left a comment

idelpivnitskiy left a comment

chemicL commented Oct 13, 2021

		private boolean appendedCatchAllFilter;

		public abstract GrpcServerBuilder initializer(HttpInitializer initializer);

Untangling GrpcServerBuilder from HttpServerBuilder #1861

Untangling GrpcServerBuilder from HttpServerBuilder #1861

Conversation

chemicL commented Oct 4, 2021

tkountis left a comment

Choose a reason for hiding this comment

tkountis Oct 5, 2021

Choose a reason for hiding this comment

chemicL Oct 5, 2021

Choose a reason for hiding this comment

idelpivnitskiy Oct 5, 2021 • edited Loading

Choose a reason for hiding this comment

chemicL Oct 7, 2021

Choose a reason for hiding this comment

chemicL commented Oct 5, 2021

tkountis left a comment

Choose a reason for hiding this comment

idelpivnitskiy Oct 5, 2021

Choose a reason for hiding this comment

chemicL Oct 7, 2021 • edited Loading

Choose a reason for hiding this comment

idelpivnitskiy commented Oct 5, 2021 • edited Loading

idelpivnitskiy left a comment

Choose a reason for hiding this comment

idelpivnitskiy left a comment

Choose a reason for hiding this comment

chemicL commented Oct 13, 2021

Untangling `GrpcServerBuilder` from `HttpServerBuilder` #1861

Untangling `GrpcServerBuilder` from `HttpServerBuilder` #1861

idelpivnitskiy Oct 5, 2021 •

edited

Loading

chemicL Oct 7, 2021 •

edited

Loading

idelpivnitskiy commented Oct 5, 2021 •

edited

Loading