Implementation of the ActorAsyncProxy by JuanJose-Herrera · Pull Request #58 · dapr/java-sdk

JuanJose-Herrera · 2019-12-24T05:33:39Z

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

Description

Please explain the changes you've made

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: #[issue number]

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

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

msftclas · 2019-12-24T05:33:52Z

All CLA requirements met.

artursouza · 2019-12-24T20:11:13Z


        post(stateUrl, state.toString()).thenAccept(response -> {
-          int resCode = response.statusCode() == 200 ? 200 : 500;
+          int resCode = response.statusCode() == 201 ? 201 : 500;


Please, support both 200 and 201.

artursouza · 2019-12-24T20:13:23Z

   * @return Asynchronous result with the Actor's response.
   */
-  Mono<String> invokeActorMethod(String actorType, String actorId, String methodName, String jsonPayload);
+  <T> Mono<T> invokeActorMethod(String methodName, Object data,  Class<T> clazz) throws JsonProcessingException;


Throws IOException

artursouza · 2019-12-24T20:13:36Z

+   * @param methodName Method name to invoke.
+   * @return Asynchronous result with the Actor's response.
+   */
+  public Mono invokeActorMethod(String methodName) ;


artursouza · 2019-12-24T20:14:03Z

+   * @param data Object with the data.
+   * @return Asynchronous result with the Actor's response.
+   */
+  public Mono invokeActorMethod(String methodName, Object data) throws JsonProcessingException;


Exception and Mono<Void>

artursouza · 2019-12-24T20:14:53Z

+   */
+  private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();
+
+  private ActorId actorId;


This class should remain stateless without awareness of actorid and actortype.

artursouza · 2019-12-24T20:19:02Z

 /**
 * Interface to invoke actor methods.
 */
 interface ActorProxyAsyncClient {


This one maps to ActorProxy in .net sdk.

Move implementation to a new ActorProxy interface. Leave this file unchanged.

artursouza · 2019-12-24T20:22:10Z

 * Http client to call actors methods.
 */
-class ActorProxyHttpAsyncClient extends AbstractDaprClient implements ActorProxyAsyncClient {
+public class ActorProxyHttpAsyncClient extends AbstractDaprClient implements ActorProxyAsyncClient {


Revert this change.
Move implementation for ActorProxy to ActorProxyImpl

artursouza · 2019-12-24T20:23:22Z

 * Requires Dapr running.
 */
-public class DaprHttpAsyncClientIT {
+public class DaprHttpAsyncClientTest {


Keep it untouched and move your tests to a new class.

Also IT means integration tests in Maven.
Test mean unit tests.

artursouza

Thanks for this PR. This first one will have more comments as you onboard into the code style and learn about previous decisions in the code (like the async call in the http client - we want to keep that).

artursouza · 2019-12-25T01:22:58Z

-            String respBodyString = responseBody.string();
-            cb.onSuccess(respBodyString);
-            response.close();
+    try (Response response = this.httpClient.newCall(request).execute()) {


Revert this change to the previous solution. This one is a blocking operation while the previous one was async.

artursouza · 2019-12-25T01:23:16Z

   */
  public final Mono<String> invokeAPI(String method, String urlString, String json) throws RuntimeException {
-
-    DaprHttpCallback cb = new DaprHttpCallback() {


Keep this and the previous async code.

artursouza · 2019-12-25T01:23:36Z

+     */
+    ActorId getActorId();
+
+


Excessive spacing.

artursouza · 2019-12-25T01:24:09Z

+import reactor.core.publisher.Mono;
+
+import java.io.IOException;
+


Javadoc this interface, specially because it is client facing.

artursouza · 2019-12-25T01:24:39Z

+     * @param clazz The type of the return class.
+     * @return Asynchronous result with the Actor's response.
+     */
+    <T> Mono<T> invokeActorMethod(String methodName, Object data,  Class<T> clazz) throws IOException;


Don't throw, just let the Mono response handle the exception async.

artursouza · 2019-12-25T01:25:48Z

+     */
+    private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();
+
+    private ActorId actorId;


artursouza · 2019-12-25T01:26:14Z

+    private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();
+
+    private ActorId actorId;
+    private String actorType;


artursouza · 2019-12-25T01:26:21Z

+
+    private ActorId actorId;
+    private String actorType;
+    private ActorProxyHttpAsyncClient abstractDaprClient;


artursouza · 2019-12-25T01:27:01Z

+
+    @Test()
+    public void constructorActorProxyTest() {
+        final ActorProxyHttpAsyncClient actorProxyAsyncClient = (ActorProxyHttpAsyncClient)new ActorProxyClientBuilder().buildAsyncClient();


Mock it, not real http communication for unit tests.

artursouza · 2019-12-25T01:27:19Z

 public class OrderManager {

  static HttpClient httpClient;
+  static final List<Integer> httpOkStatus = Arrays.asList(200,201);


HTTP_OK_STATUSES

Theoretically this is mutable, so it should be wrapped with Collections.unmodifiableList(). It is simpler to just use OR in the if condition.

artursouza · 2019-12-27T04:48:14Z

Took your changes (commits) and put some fixes and refactoring on top: #62

Fix OrderManager example in order to process http 201

963f229

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

artursouza suggested changes Dec 24, 2019

View reviewed changes

Update code with the changes proposed by Artur in the code review

2116a40

artursouza suggested changes Dec 25, 2019

View reviewed changes

artursouza closed this Dec 27, 2019

		import reactor.core.publisher.Mono;

		import java.io.IOException;

Uh oh!

Conversation

JuanJose-Herrera commented Dec 24, 2019

Description

Issue reference

Checklist

Uh oh!

msftclas commented Dec 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

artursouza 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!

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!

artursouza commented Dec 27, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

msftclas commented Dec 24, 2019 •

edited

Loading