Migrate actors to use grpc by addjuarez · Pull Request #954 · dapr/dotnet-sdk

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

Signed-off-by: addjuarez addiajuarez@gmail.com

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

Attention: Patch coverage is 1.44928% with 272 lines in your changes missing coverage. Please review.

Project coverage is 67.11%. Comparing base (1605ecd) to head (4cae4c2).
Report is 166 commits behind head on master.

Files with missing lines	Patch %	Lines
src/Dapr.Actors/DaprGrpcInteractor.cs	0.00%	208 Missing and 1 partial ⚠️
src/Dapr.Actors/DaprHttpInteractor.cs	0.00%	30 Missing ⚠️
src/Dapr.Actors/Client/ActorProxyFactory.cs	10.00%	16 Missing and 2 partials ⚠️
src/Dapr.Actors/Runtime/ActorRuntime.cs	10.00%	8 Missing and 1 partial ⚠️
src/Dapr.Actors/Runtime/ActorRuntimeOptions.cs	0.00%	3 Missing ⚠️
src/Dapr.Actors/Client/ActorProxyOptions.cs	33.33%	2 Missing ⚠️
src/Dapr.Actors/Runtime/DaprStateProvider.cs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            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.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: addjuarez <addiajuarez@gmail.com>

* feat: add metadata api Signed-off-by: saberwang <saberwang@hotmail.com> * Using HTTP API to get and set dapr metadata Signed-off-by: saberwang <saberwang@hotmail.com> * style Signed-off-by: saberwang <saberwang@hotmail.com> * using grpc to add metadata api Signed-off-by: saberwang <saberwang@hotmail.com> * style Signed-off-by: saberwang <saberwang@hotmail.com> Co-authored-by: halspang <70976921+halspang@users.noreply.github.com> Signed-off-by: addjuarez <addiajuarez@gmail.com>

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

+        /// <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

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


Nit: These should be indented.

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

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


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

+        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

            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

+                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

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


useGrpc -> UseGrpc

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

+            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

    /// <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

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


Same comments, we can remove this.

Signed-off-by: addjuarez <addiajuarez@gmail.com>

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

        }

-        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

+        /// <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

+        /// <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

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


Do we need both endpoints?

Signed-off-by: addjuarez <6789375+addjuarez@users.noreply.github.com>

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 <6789375+addjuarez@users.noreply.github.com>

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

        }

-        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

+        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

        }

-        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 <addiajuarez@gmail.com>

WhitWaldo · 2025-02-28T05:57:16Z

I'm going to go ahead and close this issue for now in light of the new proposal at the runtime for formally adding (bidirectional streaming)[https://github.com/dapr/proposals/pull/72] and PubSub support on a new gRPC actor implementation.

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 <addiajuarez@gmail.com>

Fix token tests

a0dc723

Signed-off-by: addjuarez <addiajuarez@gmail.com>

update go version

e9b18d6

Signed-off-by: addjuarez <addiajuarez@gmail.com>

Fix e2e tests

9afcb33

Signed-off-by: addjuarez <addiajuarez@gmail.com>

Fix exception test

f90d2ec

Signed-off-by: addjuarez <addiajuarez@gmail.com>

remove unused http vars

d19061c

Signed-off-by: addjuarez <addiajuarez@gmail.com>

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 <addiajuarez@gmail.com>

suppress obsolete error

a36c327

Signed-off-by: addjuarez <addiajuarez@gmail.com>

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 <6789375+addjuarez@users.noreply.github.com>

addjuarez added 5 commits December 16, 2022 21:13

excpetion test

8fcf953

Signed-off-by: addjuarez <6789375+addjuarez@users.noreply.github.com>

update test

57e7579

Signed-off-by: addjuarez <6789375+addjuarez@users.noreply.github.com>

add e2e test

484b470

Signed-off-by: addjuarez <6789375+addjuarez@users.noreply.github.com>

Fix exception test

5dff37a

Signed-off-by: addjuarez <6789375+addjuarez@users.noreply.github.com>

Fix exception test

611c4e4

Signed-off-by: addjuarez <6789375+addjuarez@users.noreply.github.com>

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 <addiajuarez@gmail.com>

rerun

09f4fd0

Signed-off-by: addjuarez <addiajuarez@gmail.com>

Merge branch 'master' into actorMigrate

4cae4c2

WhitWaldo closed this Feb 28, 2025

Conversation

addjuarez commented Sep 21, 2022

Description

Issue reference

Checklist

Uh oh!

codecov Bot commented Sep 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

halspang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addjuarez commented Dec 16, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WhitWaldo commented Feb 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov Bot commented Sep 22, 2022 •

edited

Loading