Refactoring HTTP Client and creating Client HTTP Adapter - #68
Conversation
|
This PR looks great. I'll start reviewing soon. |
| * @return a Mono plan of type Void | ||
| */ | ||
| <T> Mono<Void> publishEvent(T event); | ||
| <T> Mono<Void> publishEvent(ClientRequest<T> event); |
There was a problem hiding this comment.
This seems to make the interface specific to http
There was a problem hiding this comment.
I did not found anything specific to GPRC that is needed beyond the actual value to be sent over, unlike for http who needs more data, do you have any suggestion on how to handle that?
|
We might want to refactor a little more. Since users are not expected to use the HttpDaprClient or GrpcDaprClient directly but instead talk to the Adapter (which really acts like a facade to the Dapr API), the questions remains on why do we need wrappers on top of the autogenerated gRPC libraries and native HTTP. All of the Dapr functionality can be accessed today in gRPC except for actors. Once this issue is complete, SDKs will be able to use gRPC only and not HTTP. In addition, the DaprClientAdapter will need to host either a web-server or gRPC server for Dapr to communicate with it (for example, getting published events, binding events, etc.). Given the above, combined with simplicity and maintainability, would it not be easier to refactor to this? |
|
Even when GRPC will provide a better performance dapr will still provide
both ways to comunicate to the API, so we need to provide both ways through
a common interface for simplicity, that's why it was designed this way,
settling for GRPC for everything unless is not provided will limit the Java
SDK in comparison with the other languages SDKs which is not ideal.
The users of the SDK should be provided with both supported ways and let
them decide which one is better for them.
|
|
I disagree. The whole purpose of the SDK is to be prescriptive and simplify the life for the developer. A developer consumes an API, not a communication protocol. Telling the user to choose between two ways which are identical in features defeats the purpose of an SDK. Also, when you say "other language SDKs", remember that all of them are gRPC only except for the dotnet SDK which is headed that direction too. |
|
The "Adapter" layer is acting as a Adapter today because it is a translation between the common API to one underlying raw API. Once we do a more complex integration (grpc for one vs http for other), then it becomes a Facade since it is simplifying multiple interfaces into a single common interface. The current strategy of having one adapter per raw client is aligned to the goal of having GRPC only. In that mode, we simply stop offering the http implementation and default to GRPC only, deprecating the HTTP option in the builder. This is not in scope for this milestone. Even if we decide to implement the "facade" solution where we use GRPC for some APIs and HTTP for others - it is just another implementation of the same interface (maybe even using the two adapters created here). I do see this change in line with our goal. |
artursouza
left a comment
There was a problem hiding this comment.
I guess there is an existing over-engineering problem we should tackle at this PR - it will make it easier: we have a client for actors and for for non-actors APIs and it is overkill. I guess it is causing the problem of changing the common interface to have http-specific methods.
| return this; | ||
| } | ||
|
|
||
| public ActorProxyBuilder withOkHttpClientBuilder(OkHttpClient.Builder okHttpClientBuilder) { |
There was a problem hiding this comment.
Let's not expose OKHttp to the layer above.
There was a problem hiding this comment.
No exposing the Http builder to the above layer will force us to use a client with only default configurations, which could be not very secure.
| * @param port Port for calling Dapr. (e.g. 3500) | ||
| * @param httpClient RestClient used for all API calls in this new instance. | ||
| */ | ||
| ActorProxyHttpAsyncClient(int port, OkHttpClient httpClient) { |
There was a problem hiding this comment.
The idea is to share the same httpClient for all instances. With this change, each instance will have its own client, which is expensive.
There was a problem hiding this comment.
so for sharing the same httpClient I should be gettting the acual implementation rather than the builder?
| @@ -18,8 +17,9 @@ class AppToDaprClientBuilder extends AbstractClientBuilder { | |||
| * @return Builds an async client. | |||
| */ | |||
| public AppToDaprAsyncClient buildAsyncClient() { | |||
There was a problem hiding this comment.
This class needs to go. Any place where this class is being used, should just be DaprClientBuilder.
| * Http client to call Dapr's API for actors. | ||
| */ | ||
| //public class DaprHttpAsyncClient implements DaprAsyncClient { | ||
| class AppToDaprHttpAsyncClient extends AbstractDaprHttpClient implements AppToDaprAsyncClient { |
There was a problem hiding this comment.
Move all implementation to AbstractDaprHttpClient - there is no need for this to be separate.
There was a problem hiding this comment.
you mean move this to DaprHttp.java? this is very specific to Actors, probably rename it to ActorsHttpClient and move it package io.dapr.actors.client?
Or move the methods to the ActorProxy Client?
We don't need a wrapper layer around gRPC or HTTP. As far as I can see, they are redundant, as the only consumer of these is the Adapter layer today. So having the Adapter (should probably be only called Client) use the autogenerated gRPC client to call into Dapr for everything besides actors, and making direct HTTP calls for actors is the way to go in my opinion. It simplifies the architecture, removing unneeded interfaces and wrappers and improves maintainability by having a single implementation instead of several. |
Once we decide to have a single implementation of the common interface, yes we can simplify. |
Can we make the decision? the common interface should actually be very similar to the one currently in use, only missing the custom classes per operation (for requests and responses) instead of generics. |
|
The reason why I press on this now is because changes like these are best done sooner rather than later. We've had our fair share of technical debt in the past.. |
I remember us having this discussion about being GRPC only (except for Actors) before and it was agreed to support both in the SDKs. Let's add this to the review we will have soon. This change is not a one-way road - we can quickly switch to the GRPC+HTTP hybrid if we decide to. I am more concerned about the callback API - that will be heavily impacted if we decide on that. My point is: calling out this change now is a big deal and is not the intent for this PR. |
BTW, there is a more fundamental tech debt that I want to address here instead. I will post my detailed request soon - I am editing it now. |
Calling out this change later will be a bigger deal. If there's no reason to not go down the path of a single client using gRPC and HTTP directly, I don't see why we should insert work that we know will change later and will be more difficult to change. |
My point is the opposite: keeping both adapters now and moving to the facade later is easier than if we decide the other way around. |
First, can you explain why its easier? |
It is temporary if we assume the decision has been made - AFAIK we will discuss this in the design review next week. |
Very well. let's label this as needs-discussion and merge/reject/change after we discuss. |
This is a WIP branch, do you really think we cannot merge this? I understand that we should agree before going to master. There is more work pending on top of this change, so if we block this then one of three things might happen:
What is your expectation by blocking this PR? |
My expectation is we make the best possible choice for the project based on technical merits and agreed upon decisions, and to not continue work on building blocks that may or may not exist after being merged and are awaiting discussion. Based on what you've written, it seems like this is a major and crucial decision if most work done by the team is blocked on this, emphasizing further the importance of the architectural decision made here. This is Friday after noon. Is it really that important we merge now if we discuss this beginning of next week? |
|
Hi Andres, I guess we are having problems with the refactoring because we have two clients and this is causing confusion. Also, after talking to Yaron, we should provide a single client builder and interface. Here is my proposal: Merge ActorProxyHttpAsyncClient and AppToDaprHttpAsyncClient into a single class called DaprHttpClientAdapter Merge AppToDaprAsyncClient and ActorProxyAsyncClient into the existing DaprClient interface Merge builders into a single builder (DaprClientBuilder) that will use HTTP only – let GRPC client unused for now. If we need to support multiple flavors, we can do it easily after this change. Change code that uses the classes above need to change to reflect that. Thanks, |
|
After discussing, we agreed to:
|
0ea11c2 to
9c730b8
Compare
|
@yaron2 @artursouza already addressed the observations made, please take another look, and let me know what you think. |
artursouza
left a comment
There was a problem hiding this comment.
This is pretty good. There are some changes still needed.
| /** | ||
| * Base class for client builders | ||
| */ | ||
| public abstract class AbstractClientBuilder { |
There was a problem hiding this comment.
Forgot to delete this class, which is no longer required.
| * @throws UnsupportedOperationException every time is called. | ||
| */ | ||
| public <T> Mono<Void> invokeService(String verb, String appId, String method, T request) { | ||
| return Mono.error(new UnsupportedOperationException("Operation not supported for GRPC")); |
There was a problem hiding this comment.
Actually, this is supported.
| private T value; | ||
| private String key; | ||
| private String etag; | ||
| private StateOptions options; |
There was a problem hiding this comment.
We should not get this argument from user. If this is only for internal use, then there should be another class to represent this composition of KeyValue + StateOptions.
Also, make this class immutable. There should be no need to change it.
There was a problem hiding this comment.
That's there because it's needed like that for GRPC
There was a problem hiding this comment.
Important
This structure is the following:
- For both gRPC and HTTP (mentioned before)
- Is not internal. It's provided by the user.
| @@ -0,0 +1,13 @@ | |||
| package io.dapr.client.domain; | |||
|
|
|||
| public class StateOptions { | |||
| * Dapr's default hostname. | ||
| */ | ||
| public static final String DEFAULT_HOSTNAME = "localhost"; | ||
| public static final String DEFAULT_HOSTNAME = "http://localhost"; |
There was a problem hiding this comment.
hostname is "localhost", consumers of that constant expect it to be an actual hostname (without "http://").
"http://localhost/" should be called DEFAULT_HTTP_BASE_URL or something.
| * @param data State to be saved. | ||
| * @return Asynchronous void result. | ||
| */ | ||
| Mono<Void> saveStateTransactionally(String actorType, String actorId, String data); |
There was a problem hiding this comment.
Use Actor is every actor related method for consistency.
| public DaprClientBuilder withPort(Integer port) { | ||
| this.port = port; | ||
| return this; | ||
| public DaprClientBuilder() { |
There was a problem hiding this comment.
You probably don't need this constructor anymore.
Creating DaprClientHttpAdapter to be exposed to the users. Refactor all uses of the http Client to use the adapter instead following composition rather than inheritance. Dealing with conflicts
…onality in the same place, having a single entry point for all communications to DAPR. Leaving GRPC Adapter implemented, but without the possibility to create an instance of it. Fixing Test cases
9c730b8 to
fd6dbe2
Compare
|
Comments have been addressed, can you guys let me know if any other comment. |
* Refactoring HTTP Client to follow same pattern as the GRPC Client. Creating DaprClientHttpAdapter to be exposed to the users. Refactor all uses of the http Client to use the adapter instead following composition rather than inheritance. Dealing with conflicts * Renaming AppToDaprHttpAsync to AppToDaprHttpAsyncClient changed previously by mistake * Refactor Adapters to centralize all generic and actor specific functionality in the same place, having a single entry point for all communications to DAPR. Leaving GRPC Adapter implemented, but without the possibility to create an instance of it. Fixing Test cases * Addressing PR comments
* Basic Dapr HTTP Client for Actors. * ActorTypeInformation with parsing logic + some stubs. * Fixing license. * Adding MethodContext + StateSerializer. * Add ActorRuntime (#44) * Add ActorRuntime * split DaprAsyncClient hierarchy into 2 hierarchies for the different directions of communication * Rename DaprClientBase to AbstractDaprClient * Add ActorId * Encapsulating async call in AbstractDaprClient as a single method. (#46) * Add ActorMethodInfoMap (#47) * Add ActorMethodInfoMap * Add ConverterUtils class (#49) * Add ConverterUtils + unit test * Improved async call to okhttp. Former solution just wrapped a blocking call, now we using asynch call and collect the results via callbacks. Mono's are created based on the result. This make more efficient use of the resources since nothing is blocking now. Reformatted according new checkstyle (#48) * More reformatting to 2 spaces indentation. (#50) * Add ActorTimer and related (#51) * Add ActorTimer and related * cr * ActorService + DaprStateProvider + some more. (#53) * Faster JSON building + separate auto-gen jar + tests. (#55) * closed the response objects and returning a string (#56) * Add ReminderInfo (#54) * Add ReminderInfo * remove extra var * More Actors Stuff (#57) * ActorManager * More work done. * Adding example for actor runtime service. * Implements ActorStateManager + fixes + javadocs. * ActorProxy + some refactoring (#62) * ActorManager * More work done. * Adding example for actor runtime service. * Implements ActorStateManager + fixes + javadocs. * Fix OrderManager example in order to process http 201 Change the implement the calls to DAPR using the class AbstractDaprClient because before the change the class always return Mono.Empty Implementation of the ActorAsyncProxy Change the name of the Actor Dapr Http Async Client * Update code with the changes proposed by Artur in the code review * Changes to support ActorProxy + Fixes. Co-authored-by: Juan Jose Herrera <35985447+JuanJose-Herrera@users.noreply.github.com> * Adding new methods for Http Client (#60) * Adding new methods for Http Client * Finishing implementation for HttpClient * Adding JavaDoc and Changing return Type to new Methods * Using ObjectSerializer and changing Constants values * Adding gRPC client adapter, to encapsulate gRPC logic from the user (#61) * Adding a common adapter to be exposed to users, for calling either the gRPC or the HTTP Clients. Implementing gRPC adapter based on interface. Moving ActorStateSerializer and renamed to be used as a generic utility for Serializing objects * Creating a builder for the GrpcClient Abstracting generic serialize and deserialize methods into a generic base class while leaving Actor specific stuff into the ActorStateSerializer class Adding Javadocs * Unit tests for ActorManager + fixes for Timer and Reminder. (#63) * Add UnitTest to ActorProxyImpl and JaCoCo (coverage tool) implementation (#67) * Change from unwrapMethodResponse to deserialize, because the response from an actor method is not wrapped in "Data" object Add Unit test for the ActorProxy class * #24 Implement more testing to the ActorProxyImpl and refactor how we manage the errors with Mono #20 Add the coverage tool JaCoCo, the rules are commented at this moment in order to allow to execute the examples at this moment, if we enable the rules, the compilation fails. * Enable rules for unit test coverage with a minimum of 0 in order to allow the developers to success compile, the minimum should be set to .8 before the projects ends * Use the unwrapMethodResponse of the serilizer in order to deserilize actor responses. * Reverting ActorProxyImpl.java There is not need to change ActorProxyImpl since the previous change to not wrap it with the "data" structure was incorrect. Co-authored-by: Artur Souza <artursouza.ms@outlook.com> * Starting package to handle SpringBoot integration. (#69) * Request body must be null on get for okhttp, empty body does not work… (#70) * Request body must be null on get for okhttp, empty body does not work. Also, expose the ActorStateManager methods that ought to be exposed to user. * Make ctor package private and use equals for string compare * Refactoring HTTP Client and creating Client HTTP Adapter (#68) * Refactoring HTTP Client to follow same pattern as the GRPC Client. Creating DaprClientHttpAdapter to be exposed to the users. Refactor all uses of the http Client to use the adapter instead following composition rather than inheritance. Dealing with conflicts * Renaming AppToDaprHttpAsync to AppToDaprHttpAsyncClient changed previously by mistake * Refactor Adapters to centralize all generic and actor specific functionality in the same place, having a single entry point for all communications to DAPR. Leaving GRPC Adapter implemented, but without the possibility to create an instance of it. Fixing Test cases * Addressing PR comments * PubSub + related fixes. (#71) * Fixing issue regarding GET and DELETE method (#76) * Unit tests for PubSub + JavaDocs. (#75) * Invoke service + unit tests. (#78) * Java sdk wip (#77) * Fixing issue regarding GET and DELETE method * Adding unit test for DaprHttp.java * Adding test scope * Renaming a property and fixing conflict to merge. Co-authored-by: Artur Souza <artursouza.ms@outlook.com> * pubsub sample needs update for listener interface (#80) * Fix bindings + add samples (#81) * Fix binding and add samples * Fix sample command line * update comment for name change * lowercase d matches runtime, though it looks like it is case insensitive * Spliting Actor client and runtime into a separate jar. (#83) * Unit testing + bug fixing in GRPC Adapter (#79) * Adding mockito plugin to be able to mock final classes Increasing test coverage for ObjectSerializer Fixing bug in GRPC Adapter while creating the envelopes, found during unit testing. * First step into returning Http Headers as part of the response for the DaprClientHttpAdapter Updating State object to match the API. Fixing Broken Unit Tests and increasing coverage for DaprClientGrpcAdapter * Adding documentation, fixing typos and renaming support method to be more descriptive. * Addressing PR comments Increasing test coverage Fixing Merge conflicts * Addressing PR comments * Addressing comments regarding where to build the query parameters forthe HTTP Client calls. (#84) * Addressing comments regarding where to build the query parameters for the HTTP Client calls. * Making map immutable while also preventing a NPE * Fixing DaprHttp and its tests. (#93) * Increasing test coverage for DaprClientGrpcAdapter (#94) * Upload jacoco test coverage report to artifact storage (#95) * set jacoco report output dir * upload test report * add dapr version info * #26 Integretion Testing Initial Example (#74) * #26 Add Hello World Integration Testing working on Windows, need work to work on MAC and Linux * #26 Add new Integration Test to test DAPR state functionality * #26 Add Hello World Integration Testing working on Windows, need work to work on MAC and Linux * Update Integration Testing getting free ports automatically * #26 Refractor to use a base class for all the integration tests * #26 Make StateOptions as optional in order to not throw a null pointer exception * #26 Remove empty lines and correct the ident * Adding license to DaprIntegrationTestingRunner Co-authored-by: Young Bu Park <youngp@microsoft.com> Co-authored-by: Artur Souza <artursouza.ms@outlook.com> * Fixing and finishing unit tests (#97) * Fixing and finishing unit tests * Fixing styling Issues and adding new test * Unit tests for stateful actors + fixes. (#104) * Receiving the StateOptions on save as part of the State, to comply with the DAPR API (#105) * Receiving the StateOptions on save as part of the State, to comply with the DAPR API * #26 Add integration test for Concurrency funtionality in the states module Co-authored-by: Juan Jose Herrera <35985447+JuanJose-Herrera@users.noreply.github.com> Co-authored-by: Artur Souza <artursouza.ms@outlook.com> * Deploy Jar packages to Nexus via CI (#106) * Add SNAPSHOT suffix to version * set deployment setup * add ossrh repostiory * OSSRH setting * add deployment steps * exclude examples jar * remove condition * exclude spring boot pkg Co-authored-by: Artur Souza <artursouza.ms@outlook.com> * deactivate should not call save (#108) Co-authored-by: Artur Souza <artursouza.ms@outlook.com> * Fixing bug, where call should be deferred instead of async (#102) * Fixing bug, where call should be deferred instead of async * Removing unused pool property Returning null as response body if empty response is found. Adding Test case for validating callback is executed when expected. * Fixing assertion based on previous changes * Adding test case to verify that the call to grpc only happens when the requestor block the thread, instead of in parallel immediately after calling the methods in the adapter. Documenting complex test case * Fixing merge conflicts * Documenting complex test case * Disable GPG sign by default (#111) * [CD] Fix gpg private key import (#113) * fix gpg key import process * add GPG_TTY env setting * revert * rename dapr-sdk to dapr-actors * fix * Fix GPG plugin * [CD] Conditional SNAPSHOT and final release (#115) * Release pkg based on tag and branch * Update build.yml * Update build.yml * Creating unit tests for HttpAdapter + Adding junit 5 dependency + Reformating code (#112) Co-authored-by: Artur Souza <artursouza.ms@outlook.com> * Removing springboot jar and adding some unit tests for ActorRuntime. (#114) * Release SNAPSHOT pkg for every build (#118) * Release SNAPSHOT build every build * fix * Fix sample (#119) Co-authored-by: Artur Souza <artursouza.ms@outlook.com> * Unit tests for actors, no state. (#117) * actor unit tests, no state clean up a bit * Rename class * Fix merge Co-authored-by: Artur Souza <artursouza.ms@outlook.com> * Moving io.dapr.runtime to test + fixing examples accordingly. (#121) * mvn integration-test doesn't work w/o this plugin (#122) * Resolving conflict for PR #110 (#130) * Renaming State domain object. Removing duplicate use of State Options. Adding custom serialize and deserialize classes for StateOptions objects. * #26 Refractor StateOptions to clean unused code * Fixing merge issues. * Fixing unit test for DaprClientHttpAdapterTest after refactoring. Co-authored-by: Juan Jose Herrera <35985447+JuanJose-Herrera@users.noreply.github.com> * Refactor StateOptions and add IT consistency testing (#110) * Renaming State domain object. Removing duplicate use of State Options. Adding custom serialize and deserialize classes for StateOptions objects. * #26 Refractor StateOptions to clean unused code * Fixing merge issues. * #26 Add integration test for states using Retry policies Co-authored-by: Artur Souza <artursouza.ms@outlook.com> * Fix IT issues, to run the integration test rember to add the parameter skipITs=false (#131) * Addresses multiple items from design review (#124) * Use byte[] for customer data and serialized data. Fixes for actor demo. * Simplifying ObjectSerializer + handling CloudEvent properly. * Split DaprClient into 3 parts to hide actor APIs. * Remove Actor "dummy" interface. * Supports custom serializer. * Change seriaizer to receive Object directly. * Handling custom serialization in State APIs + enhancements to the state APIs. * Addressing small comment on Mono chain in ActorManager. * Make serializer mandatory, throwing exception when null. * Fix arg parsing problem -Dexec.args= seems to not allow hyphens inside (#136) * Fix bug in Actor due to timer serialization. (#139) * Missing block (#140) * Install local test kafka before running build and tests (#142) * local test kafka docker-compose yaml * add kafka install step * Separate serializer for state. (#135) * Add gRPC State Integration Test (#138) * Add GRP State Integration Test * Explain ignored test cases for GRPC * Explain ignored test cases for GRPC * Update DaprClientTestBuilder.java Co-authored-by: Artur Souza <artursouza.ms@outlook.com> * Actor Activation / Deactivation Integartion Test (#144) * Adding common setup for starting dapr applications in Integration Tests. Adding integration test for Actor Activation and Reactivation. * Changing the way parameters are sent to the application running under Dapr in ITs to fix issue found in Ubuntu about wrong parsing. Moving logic of the ActivationClient into the IT class * Fixing compilation issues in test due to rebase from upstream. * Addressing PR comments * Adding more unit test to reach 80% Coverage (#149) * Adding checkstyle + fix style in sdk-actos. (#146) * Update debug instructions since LB is not needed. (#150) * Add E2E testing for bindings (#148) * Add GRP State Integration Test * Explain ignored test cases for GRPC * Explain ignored test cases for GRPC * Update DaprClientTestBuilder.java * Add binding E2E testing. * Add fix binding E2E testing after merge with new changes * Remove example comments Co-authored-by: Artur Souza <artursouza.ms@outlook.com> * Adding documentation for examples (#153) Co-authored-by: Leon Mai <lemai@microsoft.com> Co-authored-by: ji11er <57505038+ji11er@users.noreply.github.com> Co-authored-by: Juan Jose Herrera <35985447+JuanJose-Herrera@users.noreply.github.com> Co-authored-by: mestizoLopez <ing.javierlg@gmail.com> Co-authored-by: Andres Robles <15348598+AndresRoblesMX@users.noreply.github.com> Co-authored-by: Young Bu Park <youngp@microsoft.com> Co-authored-by: Marcos Reyes <59033058+marcosreyes05@users.noreply.github.com>

Description
Refactoring HTTP Client to follow same pattern as the GRPC Client.
Creating DaprClientHttpAdapter to be exposed to the users.
Refactor all uses of the http Client to use the adapter instead following composition rather than inheritance.
Dealing with conflicts
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: #22
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: