Skip to content

feat: add client constructor with executor (DGRAPH-2746) #161

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

Merged
merged 1 commit into from
Nov 25, 2020

Conversation

abhimanyusinghgaur
Copy link
Contributor

@abhimanyusinghgaur abhimanyusinghgaur commented Nov 24, 2020

Fixes DGRAPH-2746
Fixes Discuss Issue

This PR adds a constructor to DgraphAsyncClient which accepts a java.util.concurrent.Executor argument to be used to process various asynchronous tasks executed by the client. This is useful in scenarios where one wants to do resource management manually, and not use the default pool provided by java.util.concurrent.ForkJoinPool.commonPool().

It also adds a similar constructor to the synchronous version of the client: DgraphClient.


This change is Reviewable

Copy link

@lhr0909 lhr0909 left a comment

Choose a reason for hiding this comment

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

Overall LGTM. I am just curious if we need to put the executor under the thenAccept usages around the login methods.

@abhimanyusinghgaur
Copy link
Contributor Author

Currently, those are synchronous computations that will be executed either by gRPC's executor (which can be configured while constructing a ManagedChannel for building DgraphStub) or by the thread which calls get() on the CompletableFuture.

Those computations being very small, there doesn't seem to be a need to make them async.

@abhimanyusinghgaur abhimanyusinghgaur merged commit 6c3f3f9 into master Nov 25, 2020
@abhimanyusinghgaur abhimanyusinghgaur deleted the abhimanyu/executor branch November 25, 2020 07:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants