Skip to content

close response objects to free resources - #52

Closed
ji11er wants to merge 5 commits into
dapr:java_sdk_wipfrom
ji11er:java_sdk_wip
Closed

ji11er wants to merge 5 commits into
dapr:java_sdk_wipfrom
ji11er:java_sdk_wip

Conversation

@ji11er

@ji11er ji11er commented Dec 17, 2019

Copy link
Copy Markdown
Contributor

Description

okhttp3 declares that Response objects needs closure to free resources, this wasn't done so far.

#48

Checklist

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

  • [x ] Code compiles correctly
  • Created/updated tests
  • [ x] Extended the documentation

ji11er and others added 5 commits December 15, 2019 09:46
…g 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

@artursouza artursouza left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We first check SpringBoot code overall style(ad-hoc) and then refer to this one if we cannot make a call based on the SpringBoot code: https://azure.github.io/azure-sdk/java_design.html#service-client-builder

*
* Provides the interface for implementation of proxy access for actor service.
*/
public interface IActorProxy {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't use the I* convention. Call ActorProxy and rename the implementation to ActorProxyImpl.

*
* @author Swen Schisler <swen.schisler@fourtytwosoft.io>
*/
public class ActorProxy implements IActorProxy {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not expose implementation, please make it package accessible only.

* implementing {@link #IActor} interfaces. The proxy object can be used used
* for client-to-actor and actor-to-actor communication.
*
* @author Swen Schisler <swen.schisler@fourtytwosoft.io>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are not adding @author in the code.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, missed that


import io.dapr.actors.ActorId;

public interface IActorProxyFactory {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, remove this interface. We will use the Builder pattern, similar to what we have in the http client.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

* Defines the interface that must be implemented for providing factory for
* creating actor request body and response body objects.
*/
public interface IActorMessageBodyFactory {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not use the I* convention for interfaces. Just the name: https://azure.github.io/azure-sdk/java_design.html#naming-patterns

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No builder or factory for model classes: https://azure.github.io/azure-sdk/java_design.html#model-classes

* has.
*
*/
public interface IActorResponseMessageBody {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This interface is not needed for non remoting invocations.

* Body for remoting requests . This contains all the parameters remoting method
* has.
*/
public interface IActorRequestMessageBody {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This interface is not needed for non remoting invocations.

}

public IActorMessageBodyFactory getActorMessageBodyFactory() {
return ActorMessageBodyFactory;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not needed for non remoting invocations.

return ActorMessageBodyFactory;
}

public void setActorMessageBodyFactory(IActorMessageBodyFactory ActorMessageBodyFactory) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not needed for non remoting invocations


/**
* Defines the interface that must be implemented for providing factory for
* creating actor request body and response body objects.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not needed for non remoting invocations

*/
package io.dapr.actors.communication.client;

public class ActorRemotingClient {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not needed for non remoting invocations

@ji11er ji11er closed this Dec 19, 2019
@ji11er
ji11er deleted the java_sdk_wip branch December 19, 2019 06:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants