Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gRPC client factory integration #23

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

czimpi
Copy link

@czimpi czimpi commented Jul 17, 2021

Currently the DgraphClient class only exposes a constructor accepting gRPC channels, which will then be used to create the gRPC clients. These channels have to be manually mantained (lifetime, etc.) which may lead to misuses (e.g. gRPC channels should not be created for each new request but rather reused, see docs).

Using the gRPC client factory integration is an alternative approach to outsource the maintenance of the channel respectively the client. To use this approach, the DgraphClient class must accept the gRPC client directly rather than channels.

The current implementation does add the client from the constructor to the list of clients which are subject to the (currently not active) dispose logic. This should not be a problem, as the logic is currently inactive, but in the future this might be an issue, as the client's lifetime management should be managed by the client factory. I personally would also remove the inactive dispose logic and leave the channel lifetime management to the caller of the DgraphClient.

Accepting the gRPC client directly using the client's constructor to enable dependency injection.
@CLAassistant
Copy link

CLAassistant commented Jul 17, 2021

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@MichelDiz
Copy link
Contributor

@czimpi can you update this PR and also sign the CLA?

{
Task<IDgraphClient> GetDgraphClient();
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a new line here.
EOF

}

}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

New line

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.

None yet

3 participants