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

api,core: Adds an Executor field to NameResolver.Args. #6238

Merged
merged 4 commits into from
Oct 14, 2019

Conversation

groakley
Copy link
Contributor

@groakley groakley commented Oct 3, 2019

Adds an Executor field to NameResolver.Args, which is optionally set on ManagedChannelBuilder. This allows NameResolver implementations to avoid creating their own thread pools if the application already manages its own pools.

Addresses #3703.

@groakley
Copy link
Contributor Author

groakley commented Oct 9, 2019

Some additional context: Before this change, there is no way to prevent the Channel returned by the OkHttpChannelBuilder from creating its own thread pools (even if executor() is used to override the default).

From the user's perspective, this means that even though the executor() override was used, "default-grpc-executor*" threads will still be started.

After this change, the NameResolver executor can be overridden using the channelbuilder, e.g.

OkHttpChannelBuilder.forAddress("foo", 123)
    .executor(executor1)
    .transportExecutor(executor2)
    .nameResolverExecutor(executor3)
    .build();

@ejona86
Copy link
Member

ejona86 commented Oct 11, 2019

I don't know why the discussion on #3703 was ignored.

We don't want an executor just for the name resolver on ManagedChannelBuilder. The reason the NameResolver needs an executor is that it does blocking calls. We have a few cases that need an executor for blocking reasons. Please change it so blockingExecutor() is the exposed API.

Also, in the Args API it should be clear the purpose of the Executor: that it is for blocking operations. The API should be getBlockingExecutor()/setBlockingExecutor().

Copy link
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

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

See previous comment

…set on ManagedChannelBuilder. This allows NameResolver implementations to avoid creating their own thread pools if the application already manages its own pools. Addresses grpc#3703.
@groakley
Copy link
Contributor Author

Renamed NameExecutorResolver->BlockingExecutor.

I use git/github about once a year to contribute to this project. Looks like I screwed up the branch trying to rebase from head. (Looks good in my fork project, totally screwed up in the remote project). Might be something that is easy to resolve, but I can also recreate this PR if that is the easiest path forward.

@ejona86
Copy link
Member

ejona86 commented Oct 11, 2019

Shoot. Did it a bit differently than I wanted. One moment.

Copy link
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

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

Looks pretty good. Just one comment, which I think will be fairly easy to address.

…ing the real Executor until it is first used.
Copy link
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

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

Thank you! I'm happy this is done.

@ejona86 ejona86 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Oct 11, 2019
@grpc-kokoro grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Oct 11, 2019
@ejona86 ejona86 merged commit adcfb3e into grpc:master Oct 14, 2019
ejona86 added a commit to ejona86/grpc-java that referenced this pull request Oct 15, 2019
These should have been present when initially added in grpc#6238, but we forgot.
ejona86 added a commit that referenced this pull request Oct 16, 2019
These should have been present when initially added in #6238, but we forgot.
@lock lock bot locked as resolved and limited conversation to collaborators Jan 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants