Add support for service client connection reuse. #1589

drmontgomery · 2022-03-08T19:46:17Z

Refactored ClientState into GrpcChannel.
Modified generated service clients to use GrpcChannel instead of GrpcClientSettings, with an apply() shim to create a GrpcChannel from GrpcClientSettings.
Added GrpChannelSpec to confirm that connection is being reused.

References #1588

raboof

This seems like a great idea, and the PR looks very good already! I think it needs some care to make sure binary compatibility is ensured, though - particularly between the generated code and the runtime library.

raboof · 2022-03-17T17:40:29Z

codegen/src/main/twirl/templates/JavaClient/Client.scala.txt

  }

  @@AkkaGrpcGenerated
  protected final static class Default@{service.name}Client extends @{service.name}Client {

-      private final ClientState clientState;


I don't think we can remove the ClientState type entirely: to achieve the binary compatibly promise we do in https://doc.akka.io/docs/akka-grpc/current/binary-compatibility.html , code generated with previous Akka gRPC versions should still work when a newer version of the runtime library is on the classpath. So I think we need to keep at least a deprecated 'wrapper' ClientState around, that will make sure 'older' generated code keeps working.

Thank you for the feedback!

As you suggested, I recreated ClientState as a wrapper around a GrpcChannel. I think it properly supports the constructor and methods that are used by older generated clients.

drmontgomery

I think the remaining MiMa errors about ClientState can be safely ignored, since they're about methods that were not used by the generated code.

I'm less certain about the change to the DefaultServerReflectionClient constructor. The documentation would steer users toward using ServerReflectionClient.apply over new DefaultServerReflectionClient, but the constructor was public. Should we add a deprecated constructor that takes GrpcClientSettings to the generated default client?

drmontgomery · 2022-03-17T21:21:22Z

runtime/src/main/scala/akka/grpc/internal/ClientState.scala

 */
+@deprecated("Kept for binary compatibility between generated code and runtime", "akka-grpc 2.1.4")


Is a deprecation annotation appropriate here?
Should version number be 2.1.4 (next patch release) or 2.2.0 (next minor release)?
Should version number include library name? I see both styles being used elsewhere in the project.

Is a deprecation annotation appropriate here?

Yes, I think so 👍

Should version number be 2.1.4 (next patch release) or 2.2.0 (next minor release)?

I think the next patch release would be best

Should version number include library name? I see both styles being used elsewhere in the project.

I see https://www.scala-lang.org/api/current/scala/deprecated.html promotes adding the library name, but TBH I don't think it's necessary - I'm OK with either

drmontgomery · 2022-03-18T17:01:31Z

After thinking about MiMa warnings a bit more, I added access modifiers to GrpcChannel and ChannelUtils to limit unintended exposure of constructors and methods. I think we could make the entire ChannelUtils object private to the akka package, but I wasn't sure if that is within scope for this PR.

I also noticed that ChannelUtils.closeCS was no longer used (and hasn't been since 31bb602), so I removed it.

raboof

Shaping up nicely! Can you add the mima exclusions for changed internal API's that you've verified are not used from old generated code?

raboof · 2022-03-23T09:14:07Z

runtime/src/main/scala/akka/grpc/GrpcChannel.scala

+import akka.grpc.internal.{ ChannelUtils, InternalChannel }
+import akka.grpc.scaladsl.Grpc
+
+final class GrpcChannel private (val settings: GrpcClientSettings, @InternalApi val internalChannel: InternalChannel)(


settings is used by the generated code, so could you mark it @InternalStableApi?

Good catch! When I wrote that, I was thinking that it could be convenient for library users if they could access the settings associated with a channel. Looking through the code, however, it looks like settings are generally only consumed and not exposed.

As you suggested, I added the InternalStableApi annotation to settings, and updated internalChannel to use it as well (instead of InternalApi).

raboof · 2022-03-23T09:15:16Z

codegen/src/main/twirl/templates/ScalaClient/Client.scala.txt

@@ -89,16 +91,18 @@ final class Default@{service.name}Client(settings: GrpcClientSettings)(implicit
    @{method.nameSafe}().invoke(in)
  }

-  override def close(): scala.concurrent.Future[akka.Done] = clientState.close()
-  override def closed: scala.concurrent.Future[akka.Done] = clientState.closed()
+  override def close(): scala.concurrent.Future[akka.Done] = channel.close()


Hmm, this seems rather tricky - so now closing one client will close the channel, potentially also making other clients unusable. Is that what we want? If so, perhaps we should document it prominently?

I agree. This is tricky and potentially surprising. Unfortunately, I think the trickiness is an unavoidable consequence of simultaneously supporting both client-owned and externally-owned channels within the same generated service client interface.

At least initially, I think reusing channels will be a power-user feature, like the client PowerApi. As such, I hope that callers who opt-in to shared channels will also understand the channel itself should be closed, rather than the clients that share it, but you're right that this should be more clearly called out in the documentation. I can add a warning to the API docs for the close methods, as well as a section about reusing channels in client/details.md.

We could also add a flag to track whether the service client created the channel, and throw an exception if close is called with an externally-owned channel. This is still a potentially surprising run-time error, but the exception would make the error immediately obvious. Do you think this would be worthwhile?

I can add a warning to the API docs for the close methods, as well as a section about reusing channels in client/details.md.

Sounds great!

We could also add a flag to track whether the service client created the channel, and throw an exception if close is called with an externally-owned channel. This is still a potentially surprising run-time error, but the exception would make the error immediately obvious. Do you think this would be worthwhile?

I think that would be valuable, but for now just having clear docs should be sufficient as well

drmontgomery · 2022-03-24T18:09:12Z

codegen/src/main/twirl/templates/ScalaClient/Client.scala.txt

+  def apply(channel: GrpcChannel)(implicit sys: ClassicActorSystemProvider): @{service.name}Client =
+    new Default@{service.name}Client(channel, isChannelOwned = false)
+
+  private class Default@{service.name}Client(channel: GrpcChannel, isChannelOwned: Boolean)(implicit sys: ClassicActorSystemProvider) extends @{service.name}Client {


Making the default service client private to the service object feels like an improvement to me. The default client was already marked final to prevent subclassing, and user should be using the apply() method on the service client object rather than creating default clients directly. Making the class private to the service client just enforces these rules more strongly, and brings the generated scala client into alignment with the generated java client.

That said, I'm still calibrating my application of the Boy Scout Rule. Let me know if this goes too far.

raboof · 2022-04-04T14:12:07Z

I'm sorry to leave this hanging, I hope to return to it later.

In the mean time, it seems like the CI jobs weren't correctly triggered - could you update/push a commit to get those going again?

drmontgomery · 2022-04-05T18:56:26Z

I added some documentation to API docs and client details page.

Ideally, I'd also like to add a code snippet in the details that shows how to create a GrpcChannel and use it to create multiple generated clients, but I wasn't sure about the best way to accomplish this.

I initially created a file similar to runtime/src/test/scala/docs/akka/grpc/client/GrpcClientSettingsCompileOnly, but I ran into a snag when I tried to define some example protobuf services for it. I created a file to define the service (runtime/src/test/protobuf/example.proto), but the ReflectiveCodeGen plug-in didn't seem to generate source for it.

I also considered annotating the relevant section of the GrpcChannelSpec test and referencing that from the documentation. This is less appealing, since the test uses two instances of the same service, which might be confusing to someone reading the documentation, and I'd rather not muddy the test by defining and implementing a second service.

Any suggestions here?

Refactored ClientState into GrpcChannel. Modified generated service clients to use GrpcChannel instead of GrpcClientSettings, with an apply() shim to create a GrpcChannel from GrpcClientSettings. Added GrpChannelSpec to confirm that connection is being reused.

Recreated ClientState as wrapper around GrpcChannel to preserve compatibility with generated clients from previous versions.

Added final modifier to GrpcChannel Made GrpcChannel constructor private to enforce usage of apply. Made ChannelUtils.create private to akka package.

Remove unused method ChannelUtils.closeCS. Fixed unintentional edits to import order.

…hannel.

Moved default service client into service object and made it private. Removed companion object for default service client.

raboof · 2022-08-23T09:09:11Z

I added some documentation to API docs and client details page.

Looks great!

Ideally, I'd also like to add a code snippet in the details that shows how to create a GrpcChannel and use it to create multiple generated clients, but I wasn't sure about the best way to accomplish this.

I initially created a file similar to runtime/src/test/scala/docs/akka/grpc/client/GrpcClientSettingsCompileOnly, but I ran into a snag when I tried to define some example protobuf services for it. I created a file to define the service (runtime/src/test/protobuf/example.proto), but the ReflectiveCodeGen plug-in didn't seem to generate source for it.

I also considered annotating the relevant section of the GrpcChannelSpec test and referencing that from the documentation. This is less appealing, since the test uses two instances of the same service, which might be confusing to someone reading the documentation, and I'd rather not muddy the test by defining and implementing a second service.

Any suggestions here?

I agree this would be nice, and that it would be best to show the feature with two different services. Not sure why ReflectiveCodeGen wouldn't be picking up your proto, perhaps it doesn't look at the test scope? Moving the sample code to the akka-grpc-integration-tests project might help?

I think the feature is already very attractive, though, so I intend to merge this PR as-is - we can add a code example in a later PR.

Thanks a lot!

probot-autolabeler bot added the codegen label Mar 8, 2022

drmontgomery force-pushed the WIP/grpcChannel branch from 911bd42 to 10a5ac4 Compare March 9, 2022 21:26

raboof reviewed Mar 17, 2022

View reviewed changes

drmontgomery force-pushed the WIP/grpcChannel branch 2 times, most recently from 6966c54 to 11e85a1 Compare March 17, 2022 21:02

drmontgomery commented Mar 17, 2022

View reviewed changes

raboof reviewed Mar 23, 2022

View reviewed changes

drmontgomery force-pushed the WIP/grpcChannel branch from 0e1b6f5 to 404b4a2 Compare March 24, 2022 08:31

drmontgomery commented Mar 24, 2022

View reviewed changes

drmontgomery force-pushed the WIP/grpcChannel branch from a3e4441 to 7986ed9 Compare April 5, 2022 17:50

drmontgomery force-pushed the WIP/grpcChannel branch from 7b44ee7 to e8a15e0 Compare April 12, 2022 19:18

drmontgomery added 13 commits May 9, 2022 10:05

Fix binary compatibility.

53cac21

Recreated ClientState as wrapper around GrpcChannel to preserve compatibility with generated clients from previous versions.

Add access modifiers.

668cdc8

Added final modifier to GrpcChannel Made GrpcChannel constructor private to enforce usage of apply. Made ChannelUtils.create private to akka package.

Cleanup.

8181370

Remove unused method ChannelUtils.closeCS. Fixed unintentional edits to import order.

Added MiMa exclusions.

d792e8d

Updated deprecation annotation to next patch release.

4d8c519

Added private modifier to ClientState primary constructor.

9eda273

Added InternalStableApi to parameters of primary constructor in GrpcC…

48bcfd5

…hannel.

Refactored Scala generated service client.

b1acbb9

Moved default service client into service object and made it private. Removed companion object for default service client.

Add exception when client close() is called with a shared channel.

ed79236

Added documentation.

75730b1

Added missing header.

b8acaad

Added API docs for GrpcChannel.

d4617f2

drmontgomery force-pushed the WIP/grpcChannel branch from e8a15e0 to d4617f2 Compare May 9, 2022 17:15

raboof approved these changes Aug 23, 2022

View reviewed changes

raboof merged commit b6d821e into akka:main Aug 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for service client connection reuse. #1589

Add support for service client connection reuse. #1589

drmontgomery commented Mar 8, 2022

raboof left a comment

raboof Mar 17, 2022

drmontgomery Mar 17, 2022

drmontgomery left a comment

drmontgomery Mar 17, 2022

raboof Mar 18, 2022

drmontgomery commented Mar 18, 2022 •

edited

Loading

raboof left a comment

raboof Mar 23, 2022

drmontgomery Mar 24, 2022

raboof Mar 23, 2022

drmontgomery Mar 24, 2022

raboof Mar 24, 2022

drmontgomery Mar 24, 2022

raboof commented Apr 4, 2022

drmontgomery commented Apr 5, 2022

raboof commented Aug 23, 2022

		*/
		@deprecated("Kept for binary compatibility between generated code and runtime", "akka-grpc 2.1.4")

Add support for service client connection reuse. #1589

Add support for service client connection reuse. #1589

Conversation

drmontgomery commented Mar 8, 2022

raboof left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drmontgomery left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drmontgomery commented Mar 18, 2022 • edited Loading

raboof left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raboof commented Apr 4, 2022

drmontgomery commented Apr 5, 2022

raboof commented Aug 23, 2022

drmontgomery commented Mar 18, 2022 •

edited

Loading