Skip to content

cronet: allow application to provide all threads #4249

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 3 commits into from
Mar 21, 2018

Conversation

dapengzhang0
Copy link
Member

@dapengzhang0 dapengzhang0 commented Mar 21, 2018

This is the cronet counterpart of #4139 and #4150.

@dapengzhang0 dapengzhang0 force-pushed the cronettimer branch 2 times, most recently from ab38b4c to c84e86a Compare March 21, 2018 20:08
@Override
protected final ClientTransportFactory buildTransportFactory() {
return new CronetTransportFactory(
new TaggingStreamFactory(
cronetEngine, trafficStatsTagSet, trafficStatsTag, trafficStatsUidSet, trafficStatsUid),
MoreExecutors.directExecutor(),
MoreExecutors.directExecutor(), scheduledExecutorService,
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be two separate lines

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.


import io.grpc.CallOptions;
import io.grpc.Metadata;
import io.grpc.MethodDescriptor;
import io.grpc.cronet.CronetChannelBuilder.CronetTransportFactory;
import io.grpc.internal.ClientTransportFactory;
import io.grpc.internal.ConnectionClientTransport;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this used?

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed.

assertSame(builder, builder1);

ClientTransportFactory clientTransportFactory = builder1.buildTransportFactory();

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: unnecessary empty line

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member Author

@dapengzhang0 dapengzhang0 left a comment

Choose a reason for hiding this comment

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

Thanks for the review.

@Override
protected final ClientTransportFactory buildTransportFactory() {
return new CronetTransportFactory(
new TaggingStreamFactory(
cronetEngine, trafficStatsTagSet, trafficStatsTag, trafficStatsUidSet, trafficStatsUid),
MoreExecutors.directExecutor(),
MoreExecutors.directExecutor(), scheduledExecutorService,
Copy link
Member Author

Choose a reason for hiding this comment

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

Done.


import io.grpc.CallOptions;
import io.grpc.Metadata;
import io.grpc.MethodDescriptor;
import io.grpc.cronet.CronetChannelBuilder.CronetTransportFactory;
import io.grpc.internal.ClientTransportFactory;
import io.grpc.internal.ConnectionClientTransport;
Copy link
Member Author

Choose a reason for hiding this comment

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

Removed.

assertSame(builder, builder1);

ClientTransportFactory clientTransportFactory = builder1.buildTransportFactory();

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@dapengzhang0 dapengzhang0 merged commit ca55b6f into grpc:master Mar 21, 2018
@dapengzhang0 dapengzhang0 deleted the cronettimer branch May 2, 2018 17:14
@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019
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.

2 participants