Fix in-process channel target doc

[Meijuh]

I finally found some time to test the changes in #187 and check the docs from #194. The implementation works, however this PR shows there is a mismatch between the docs at https://github.com/spring-projects/spring-grpc/blob/main/spring-grpc-docs/src/main/antora/modules/ROOT/pages/server.adoc#inprocess-server and the implementation at https://github.com/spring-projects/spring-grpc/blob/main/spring-grpc-core/src/main/java/org/springframework/grpc/client/InProcessGrpcChannelFactory.java#L54: heading // should not be part of the target. I am assuming the docs are wrong, as it seems this existing approach is followed: https://yidongnan.github.io/grpc-spring-boot-starter/en/server/configuration.html.

I have a question about the chosen approach though. By specifying in-process as a scheme, it seems as if this is a valid name resolver compliant scheme, but I could not find it at https://grpc.io/docs/guides/custom-name-resolution, nor anywhere else as far as I can see. Also I could not find a custom NameResolverProvider (https://grpc.github.io/grpc-java/javadoc/io/grpc/NameResolverProvider.html) that resolves the in-process scheme. This means in-process only exists somewhat as a scheme in spring-grpc but not in the gRPC ecosystem. This is just an observation and might confuse other devs; it could be the right approach however.

As an aside; this PR also includes a fix for a typo in the docs of methods supports(String target).

Another question that I have; with the added support for in-process servers; is it possible to exclude binding the "Health" service? I would never perform health checks on in-process servers.

[onobc]

Hi @Meijuh

Thanks for taking the time to review the changes and give this feedback. I have grouped my replies by "topic/area".

Impl concerns / doc mismatch

I believe the disconnect here is that the supports method is only called by the CompositeGprcChannelFactory and then the base DefaultGrpcChannelFactory calls into the factory newChannelBuilder which then in this case strips the in-process: prefix from the target prior to passing into the InProcessChannelBuilder.forName.

This means in-process only exists somewhat as a scheme in spring-grpc but not in the gRPC ecosystem. This is just an observation and might confuse other devs; it could be the right approach however.

Right, this is not a scheme known to gRPC core nor gRPC Java but only to gRPC ecosystem and Spring gRPC.

Note

Here is the legend of names to repo as they are in my brain:

• gRPC core -> https://github.com/grpc/grpc
• gRPC Java -> https://github.com/grpc/grpc-java
• gRPC Ecosystem -> https://github.com/grpc-ecosystem/grpc-spring
• Spring gRPC -> https://github.com/spring-projects/spring-grpc

The name gRPC Ecosystem for the previous starter repo is a very broad name. Me and you (based on your description) think of the "ecosystem" as the core plus all of its implementations, etc.. But we are on the same page.

We think this is a fair approach because:

1. the gRPC Ecosystem has heavy usage and been around a while (i.e. precedence has been set / user's are probably used to using this in-process:) and
2. the other requirements we were after (composite switching to other factories), this approach "checked the most boxes" during implementation.

PTAL and let us know if this eases your concerns. Either way, these changes are correct and this PR is needed. I will merge once we close our discussion though.

Filtering out services

Another question that I have; with the added support for in-process servers; is it possible to exclude binding the "Health" service? I would never perform health checks on in-process servers.

Ahh, great point/catch. Looks like this is not currently possible.

We did solve this for the client interceptors by introducing a ClientInterceptorFilter. However, on the server-side the only hook we have is ServerBuilderCustomizer but by the time they are applied the services have already been added to the builder.

I have created #207 and marked it with milestone 0.9.0 which we are releasing soon. If you would like to contribute this improvement it would be most welcomed. However, if you don't have time to implement this soon just lmk asap so I can see if I can squeeze it in on my side.

Thanks again.

-chris

[onobc]

@Meijuh - looks like our friend @therepanic is interested in implementing #207 !

[onobc]

@Meijuh do you mind signing your commit so the DCO will pass the checks?

[Meijuh]

@onobc Friday I already did a force push to include signoffs to my commits, but it seems the DCO check does not pick up the force push (or restart the check). Here https://github.com/Meijuh/spring-grpc/commits/main/ you can see my top 2 commits have been signed off. Any idea what I can do about this?

About using in-process as a scheme; if this is what users are expecting, then this is perfectly okay.

[onobc]

@onobc Friday I already did a force push to include signoffs to my commits, but it seems the DCO check does not pick up the force push (or restart the check). Here https://github.com/Meijuh/spring-grpc/commits/main/ you can see my top 2 commits have been signed off. Any idea what I can do about this?

About using in-process as a scheme; if this is what users are expecting, then this is perfectly okay.

Hi @Meijuh , I think you may need to rebase as well for it to pickup the changes. Take a peek at the "details" of the DCO failure and it gives some info on the rebase. If that does not work then I will dig in further. Thanks

[Meijuh]

@onobc it should be good now, there was a mismatch between the email I used for signing off, and committing.

[onobc]

Thanks for the contribution @Meijuh - much appreciated. Closing in favor of 378c1d0. I also added a follow on commit as I noticed after the review that we also needed to update the client.adoc here.