Use DaprChannel instead of closeable ActorProxyBuilder.

[artursouza]

Description

This change creates a class called DaprChannel for actor communication in Dapr. It will avoid making ActorProxyBuilder a closeable class - it was odd to have a builder class that could not be disposed. Now, DaprChannel is the class where users must keep a reference and close in the end. If we decide to use HTTP instead of gRPC, the DaprChannel still holds the Client reference, making it an effective singleton.

Summary:

• An instance of ActorProxyBuilder is now disposable and does not need to exist while DaprClient is being used.
• DaprChannel holds the "channel" to Dapr and must be referenced while app is using Dapr Actor APIs.
• DaprChannel is closeable, so users must close it after done with using Dapr Actor APIs.
• DaprChannel is encourage to be a singleton in the app, creating many instances can trash the application.
• DaprChannel is specific to Actor APIs because DaprClient in the regular SDKs is already closeable and must be reused.

Issue reference

We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.

Please reference the issue this PR will close: None

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

• Code compiles correctly
• Created/updated tests
• Extended the documentation

RELEASE NOTE: REFACTOR Created DaprChannel for actor APIs, making ActorProxyBuilder not closeable anymore.

[artursouza]

This is why this is a breaking change. This is only applicable for Actors because the proxy is per actor instance, so there must be a "client" shared some how. I think having this separate class is more intuitive then making the ActorProxyBuilder to own the channel behind the scenes.

[rynowak]

Two questions about this approach...

---

I assume that it's not feasible to make the generated proxies (actor in this example) implement Closable because it's an interface that the user provided and may not include it.

So having the user do the equivalent of (my Java is rusty, be nice!):

if (actor instanceof Closable) {
    ((Closable)actor).Close()
}

Just sounds terrible from a usability standpoint. A beginner would not understand what to do.

Equally bad is requiring the actor interface to implement Closable because then they end up with a do-nothing implementation of that interface on the server, since the proxy interface is shared.

---

Second question.

Have you considered using a static instance of DaprChannel and making the constructor arg optional via overloading? This way by default users that are not bothered about resource management can share a single channel instance that's process-scoped and can opt-in to more complexity if they want eager resource cleanup.

---

I also considered the approach of returning a handle type, which would not only be a massive breaking change, but it would also require ref-counting (yuck).

[artursouza]

First question, making the proxy or the class user provided to be closeable does not make sense because the resource to be closed (grpcChannel) is shared and reused. So a single instance of ActorProxy cannot close it because there might be other ActorProxies still using it.

The idea is to give users freedom of where to create and when to close the managed resource. If I create one for them. Having a static instance will also load it even when users don't use it. Imagine having this managed channel created for the application that just serves the actor runtime without calling into other actors? Users are also used to do this in Springboot - actually being opinionated about resource management probably belongs to the springboot package we have instead of here.

No ref counting :)

[rynowak]

Users are also used to do this in Springboot - actually being opinionated about resource management probably belongs to the springboot package we have instead of here.

So does this mean we have a recipe for using these types in a DI-enabled way? Or is that up the user to figure out?

[rynowak]

Actual code changes look straightforward, left a few questions ...

[artursouza]

I made ActorClient not implement the DaprClient interface anymore since we don't want to expose that method to applications but made it implement a method with same signature and delegate to DaprClient instead. Now, apps only see the close() method for ActorClient which is a more desirable surface area.

[mukundansundar]

left some comments.

[codecov]

Codecov Report

Merging #451 (42669e1) into master (befed4a) will increase coverage by 0.13%.
The diff coverage is 74.28%.

@@             Coverage Diff              @@
##             master     #451      +/-   ##
============================================
+ Coverage     82.80%   82.94%   +0.13%     
- Complexity      899      903       +4     
============================================
  Files            76       77       +1     
  Lines          2640     2644       +4     
  Branches        263      264       +1     
============================================
+ Hits           2186     2193       +7     
+ Misses          325      321       -4     
- Partials        129      130       +1     

Impacted Files	Coverage Δ	Complexity Δ
...c/main/java/io/dapr/actors/client/ActorClient.java	59.09% <59.09%> (ø)	6.00 <6.00> (?)
.../java/io/dapr/actors/client/ActorProxyBuilder.java	80.00% <100.00%> (+16.58%)	9.00 <5.00> (-2.00)	⬆️
...ain/java/io/dapr/actors/client/ActorProxyImpl.java	88.37% <100.00%> (ø)	27.00 <5.00> (ø)
.../src/main/java/io/dapr/client/DaprHttpBuilder.java	92.85% <100.00%> (+11.60%)	5.00 <0.00> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update befed4a...42669e1. Read the comment docs.