Migrate actors to use grpc #954

addjuarez · 2022-09-21T21:44:04Z

Signed-off-by: addjuarez [email protected]

Description

Migrate .net actor implementation to use grpc by default.

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: #486

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

codecov · 2022-09-22T00:55:27Z

Codecov Report

Merging #954 (4cae4c2) into master (1605ecd) will decrease coverage by 3.06%.
The diff coverage is 1.44%.

@@            Coverage Diff             @@
##           master     #954      +/-   ##
==========================================
- Coverage   70.17%   67.11%   -3.06%     
==========================================
  Files         160      161       +1     
  Lines        5284     5526     +242     
  Branches      567      592      +25     
==========================================
+ Hits         3708     3709       +1     
- Misses       1442     1679     +237     
- Partials      134      138       +4

Flag	Coverage Δ
net6	`67.02% <1.44%> (-3.06%)`	⬇️
net7	`67.02% <1.44%> (-3.06%)`	⬇️
netcoreapp3.1	`67.08% <1.44%> (-3.06%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/Dapr.Actors/DaprGrpcInteractor.cs	`0.00% <0.00%> (ø)`
src/Dapr.Actors/DaprHttpInteractor.cs	`44.79% <0.00%> (-7.04%)`	⬇️
src/Dapr.Actors/Runtime/ActorRuntimeOptions.cs	`72.34% <0.00%> (-4.94%)`	⬇️
src/Dapr.Actors/Runtime/DaprStateProvider.cs	`17.39% <0.00%> (+9.84%)`	⬆️
src/Dapr.Actors/Client/ActorProxyFactory.cs	`47.61% <10.00%> (-35.72%)`	⬇️
src/Dapr.Actors/Runtime/ActorRuntime.cs	`75.00% <10.00%> (-9.00%)`	⬇️
src/Dapr.Actors/Client/ActorProxyOptions.cs	`66.66% <33.33%> (-11.12%)`	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Signed-off-by: addjuarez <[email protected]>

* feat: add metadata api Signed-off-by: saberwang <[email protected]> * Using HTTP API to get and set dapr metadata Signed-off-by: saberwang <[email protected]> * style Signed-off-by: saberwang <[email protected]> * using grpc to add metadata api Signed-off-by: saberwang <[email protected]> * style Signed-off-by: saberwang <[email protected]> Co-authored-by: halspang <[email protected]> Signed-off-by: addjuarez <[email protected]>

halspang · 2022-10-03T18:32:46Z

src/Dapr.Actors/Client/ActorProxyOptions.cs

+        /// <summary>
+        /// Option to use GRPC or HTTP
+        /// </summary>
+        public bool useGrpc { get; set; } = true;


Nit: Missing whitespace at the top and public members are capitalized. useGrpc -> UseGrpc.

halspang · 2022-10-03T18:33:27Z

src/Dapr.Actors/Client/ActorProxyFactory.cs

+                .UseGrpcEndpoint(options.GrpcEndpoint)
+                .UseDaprApiToken(options.DaprApiToken)
+                .UseGrpcChannelOptions(options.GrpcChannelOptions)
+                .Build();          


Nit: These should be indented.

halspang · 2022-10-03T18:33:40Z

src/Dapr.Actors/Client/ActorProxyFactory.cs

+                .UseGrpcEndpoint(options.GrpcEndpoint)
+                .UseDaprApiToken(options.DaprApiToken)
+                .UseGrpcChannelOptions(options.GrpcChannelOptions)
+                .Build();                  


halspang · 2022-10-03T20:10:50Z

src/Dapr.Actors/DaprGrpcInteractor.cs

+        public Task SaveStateTransactionallyAsync(string actorType, string actorId, string data, CancellationToken cancellationToken = default)
+        {
+            return null;
+        }


Why did we decide to do this and create a new method instead of just using the gRPC client here?

halspang · 2022-10-03T20:11:12Z

src/Dapr.Actors/DaprHttpInteractor.cs

@@ -75,6 +76,11 @@ HttpRequestMessage RequestFunc()
            return stringResponse;
        }

+        public Task SaveStateTransactionallyAsyncGrpc(string actorType, string actorId, List<Autogenerated.TransactionalActorStateOperation> data, CancellationToken cancellationToken = default)


As commented above, I don't think we need to distinguish this.

halspang · 2022-10-03T20:31:12Z

src/Dapr.Actors/Runtime/ActorRuntime.cs

+                daprInteractor = new DaprInteractorBuilder()
+                    .UseGrpcEndpoint(options.GrpcEndpoint)
+                    .UseDaprApiToken(options.DaprApiToken)
+                    .UseGrpcChannelOptions(options.GrpcChannelOptions)
+                    .Build();                           


Needs an indent but I also feel it shouldn't use a builder.

If you do want to use a builder, I think it should take in the options.UseGrpc field and return the appropriate interactor. The benefit of that being an interface is that we don't need to know the actual implementation.

halspang · 2022-10-03T20:32:51Z

src/Dapr.Actors/Runtime/ActorRuntimeOptions.cs

+        /// <summary>
+        /// Option to use GRPC or HTTP
+        /// </summary>
+        public bool useGrpc { get; set; } = true;


useGrpc -> UseGrpc

halspang · 2022-10-03T20:36:54Z

src/Dapr.Actors/Runtime/DaprStateProvider.cs

+            if (this.daprInteractor.GetType().Name.Equals("DaprGrpcInteractor")){
+                await this.DoStateChangesTransactionallyAsyncGrpc(actorType, actorId, stateChanges, cancellationToken);
+            } else {
+                await this.DoStateChangesTransactionallyAsyncHttp(actorType, actorId, stateChanges, cancellationToken);
+            }


The interface of the interactor makes this unnecessary. Whatever the underlying object is, we'll use its method. So no need to separate them like this.

halspang · 2022-10-03T20:37:57Z

src/Dapr.Actors/Runtime/TimerInfo.cs

@@ -24,7 +24,6 @@ namespace Dapr.Actors.Runtime
    /// <summary>
    /// Represents the details of the timer set on an Actor.
    /// </summary>
-    [Obsolete("This class is an implementation detail of the framework and will be made internal in a future release.")]


Did we make this internal? I don't think we did. And if we did, that's a breaking change so we shouldn't just do it. Any reason why you removed this?

halspang · 2022-10-03T20:38:09Z

src/Dapr.Actors/Runtime/DaprStateProvider.cs

+            }
+        }
+
+        private async Task DoStateChangesTransactionallyAsyncGrpc(string actorType, string actorId, IReadOnlyCollection<ActorStateChange> stateChanges, CancellationToken cancellationToken = default)


Same comments, we can remove this.

Signed-off-by: addjuarez <[email protected]>

halspang

How hard would it be to replicate any tests that the DaprHttpInteractor has for the DaprGrpcInteractor?
It would also be good to make a base e2e test for the actors to ensure this works correctly.

halspang · 2022-11-28T15:57:13Z

src/Dapr.Actors/Runtime/DaprStateProvider.cs

        }

-        private async Task DoStateChangesTransactionallyAsync(string actorType, string actorId, IReadOnlyCollection<ActorStateChange> stateChanges, CancellationToken cancellationToken = default)
+        public async Task DoStateChangesTransactionallyAsyncGrpc(string actorType, string actorId, IReadOnlyCollection<ActorStateChange> stateChanges, CancellationToken cancellationToken = default)


Do we still need the Grpc and Http methods here? I thought with the interactors we wouldn't need these anymore.

For this method we do. The interface wants a ActorStateChange objects but grpc requires a proto object here.

halspang · 2022-11-28T15:58:02Z

src/Dapr.Actors/Client/ActorProxyOptions.cs

+        /// <summary>
+        /// Option to use GRPC or HTTP
+        /// </summary>
+        public bool UseGrpc { get; set; } = true;


This should default to false otherwise we'll change everyone's actor functionality without them opting into it.

halspang · 2022-11-28T15:58:14Z

src/Dapr.Actors/Runtime/ActorRuntimeOptions.cs

+        /// <summary>
+        /// Option to use GRPC or HTTP
+        /// </summary>
+        public bool UseGrpc { get; set; } = true;


Same here, this should be false.

halspang · 2022-11-28T15:58:34Z

test/Dapr.E2E.Test/Actors/E2ETests.ExceptionTests.cs

+            {
+                UseGrpc = false,
+                HttpEndpoint = this.HttpEndpoint, 
+                GrpcEndpoint = this.GrpcEndpoint


Do we need both endpoints?

Signed-off-by: addjuarez <[email protected]>

addjuarez · 2022-12-16T21:08:04Z

How hard would it be to replicate any tests that the DaprHttpInteractor has for the DaprGrpcInteractor? It would also be good to make a base e2e test for the actors to ensure this works correctly.

I can make e2e tests for the grpcInteractor but the unit tests for grpc are a bit more complex.

Signed-off-by: addjuarez <[email protected]>

halspang · 2023-01-06T22:19:07Z

src/Dapr.Actors/DaprHttpInteractor.cs

@@ -75,7 +77,7 @@ HttpRequestMessage RequestFunc()
            return stringResponse;
        }

-        public Task SaveStateTransactionallyAsync(string actorType, string actorId, string data, CancellationToken cancellationToken = default)
+        public Task SaveStateTransactionallyAsync(string actorType, string actorId, string data, CancellationToken cancellationToken = default, List<Autogenerated.TransactionalActorStateOperation> grpcData = default)


Is grpcData used?

Yes the grpcInteractor uses this for its data. It doesnt use the string argument

halspang · 2023-01-06T22:19:30Z

src/Dapr.Actors/DaprHttpInteractor.cs

+        public Task DoStateChangesTransactionallyAsync(DaprStateProvider provider, string actorType, string actorId, IReadOnlyCollection<ActorStateChange> stateChanges, CancellationToken cancellationToken = default)
+        {
+            return provider.DoStateChangesTransactionallyAsyncHttp(actorType, actorId, stateChanges, cancellationToken);


I thought we wouldn't need to split them up between http and gRPC anymore.

halspang · 2023-01-06T22:21:28Z

src/Dapr.Actors/Runtime/DaprStateProvider.cs

        }

-        private async Task DoStateChangesTransactionallyAsync(string actorType, string actorId, IReadOnlyCollection<ActorStateChange> stateChanges, CancellationToken cancellationToken = default)
+        public async Task DoStateChangesTransactionallyAsyncGrpc(string actorType, string actorId, IReadOnlyCollection<ActorStateChange> stateChanges, CancellationToken cancellationToken = default)


Moving this logic into the daprGrpcInteractor would remove the need to distinguish between http/grpc right?

Signed-off-by: addjuarez <[email protected]>

addjuarez marked this pull request as ready for review September 22, 2022 21:04

addjuarez requested review from a team as code owners September 22, 2022 21:04

addjuarez and others added 7 commits September 26, 2022 17:47

migrate actors to grpc

fe781b4

Signed-off-by: addjuarez <[email protected]>

Fix token tests

a0dc723

Signed-off-by: addjuarez <[email protected]>

update go version

e9b18d6

Signed-off-by: addjuarez <[email protected]>

Fix e2e tests

9afcb33

Signed-off-by: addjuarez <[email protected]>

Fix exception test

f90d2ec

Signed-off-by: addjuarez <[email protected]>

remove unused http vars

d19061c

Signed-off-by: addjuarez <[email protected]>

addjuarez force-pushed the actorMigrate branch from 475a087 to 593786f Compare September 26, 2022 17:48

Merge branch 'master' into actorMigrate

510517c

halspang requested changes Oct 3, 2022

View reviewed changes

addjuarez added 4 commits October 5, 2022 20:35

refactor

db26e1d

Signed-off-by: addjuarez <[email protected]>

suppress obsolete error

a36c327

Signed-off-by: addjuarez <[email protected]>

Merge branch 'master' into actorMigrate

1ffc9c1

Merge branch 'master' into actorMigrate

732d827

addjuarez requested a review from halspang October 28, 2022 21:01

addjuarez added 2 commits October 31, 2022 13:41

Merge branch 'master' into actorMigrate

eb8ac80

Merge branch 'master' into actorMigrate

c46b656

halspang requested changes Nov 28, 2022

View reviewed changes

change bools

a45d428

Signed-off-by: addjuarez <[email protected]>

addjuarez added 5 commits December 16, 2022 21:13

excpetion test

8fcf953

Signed-off-by: addjuarez <[email protected]>

update test

57e7579

Signed-off-by: addjuarez <[email protected]>

add e2e test

484b470

Signed-off-by: addjuarez <[email protected]>

Fix exception test

5dff37a

Signed-off-by: addjuarez <[email protected]>

Fix exception test

611c4e4

Signed-off-by: addjuarez <[email protected]>

halspang requested changes Jan 6, 2023

View reviewed changes

addjuarez and others added 3 commits January 9, 2023 12:51

refactor

62fcf04

Signed-off-by: addjuarez <[email protected]>

rerun

09f4fd0

Signed-off-by: addjuarez <[email protected]>

Merge branch 'master' into actorMigrate

4cae4c2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate actors to use grpc #954

Migrate actors to use grpc #954

addjuarez commented Sep 21, 2022

codecov bot commented Sep 22, 2022 •

edited

Loading

halspang Oct 3, 2022

halspang Oct 3, 2022

halspang Oct 3, 2022

halspang Oct 3, 2022

halspang Oct 3, 2022

halspang Oct 3, 2022

halspang Oct 3, 2022

halspang Oct 3, 2022

halspang Oct 3, 2022

halspang Oct 3, 2022

halspang left a comment

halspang Nov 28, 2022

addjuarez Dec 16, 2022

halspang Nov 28, 2022

halspang Nov 28, 2022

halspang Nov 28, 2022

addjuarez commented Dec 16, 2022

halspang Jan 6, 2023

addjuarez Jan 27, 2023

halspang Jan 6, 2023

halspang Jan 6, 2023

Migrate actors to use grpc #954

Are you sure you want to change the base?

Migrate actors to use grpc #954

Conversation

addjuarez commented Sep 21, 2022

Description

Issue reference

Checklist

codecov bot commented Sep 22, 2022 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

halspang 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

addjuarez commented Dec 16, 2022

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

codecov bot commented Sep 22, 2022 •

edited

Loading