Skip to content
This repository has been archived by the owner on Sep 26, 2023. It is now read-only.

grpc: implement channel affinity #494

Merged
merged 3 commits into from
Mar 15, 2018
Merged

grpc: implement channel affinity #494

merged 3 commits into from
Mar 15, 2018

Conversation

pongad
Copy link
Contributor

@pongad pongad commented Mar 12, 2018

@codecov-io
Copy link

codecov-io commented Mar 12, 2018

Codecov Report

Merging #494 into master will decrease coverage by 0.06%.
The diff coverage is 72%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #494      +/-   ##
============================================
- Coverage      70.5%   70.43%   -0.07%     
- Complexity      810      813       +3     
============================================
  Files           165      165              
  Lines          3773     3781       +8     
  Branches        289      293       +4     
============================================
+ Hits           2660     2663       +3     
- Misses          990      992       +2     
- Partials        123      126       +3
Impacted Files Coverage Δ Complexity Δ
...va/com/google/api/gax/grpc/GrpcDirectCallable.java 87.5% <100%> (-4.81%) 2 <0> (-1)
...main/java/com/google/api/gax/grpc/ChannelPool.java 28.2% <66.66%> (+0.42%) 5 <3> (+1) ⬆️
.../java/com/google/api/gax/grpc/GrpcCallContext.java 73.19% <71.42%> (-0.14%) 32 <5> (+2)
.../java/com/google/api/gax/grpc/GrpcClientCalls.java 83.33% <75%> (-5.56%) 3 <0> (+1)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b4eae66...e08075f. Read the comment docs.

@igorbernstein2
Copy link
Contributor

Whoa, that was fast!

ManagedChannel getChannel(int affinity) {
int index = affinity % channels.size();
index = Math.abs(index);
// If index is the most negative int, abs(index) is still negative.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

* reverse is not true: Two calls with different affinities might return the same channel.
* However, the implementation should attempt to spread load evenly.
*/
ManagedChannel getChannel(int affinity) {

This comment was marked as spam.

This comment was marked as spam.

@@ -63,26 +63,29 @@
private final CallOptions callOptions;
@Nullable private final Duration streamWaitTimeout;
@Nullable private final Duration streamIdleTimeout;
@Nullable private final Integer channelAffinity;

This comment was marked as spam.

@@ -57,6 +58,11 @@
CallOptions callOptions = grpcContext.getCallOptions();
Preconditions.checkNotNull(callOptions);

return grpcContext.getChannel().newCall(descriptor, callOptions);
Channel channel = grpcContext.getChannel();
if (grpcContext.getChannelAffinity() != null && channel instanceof ChannelPool) {

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

Copy link
Contributor

@vam-google vam-google left a comment

Choose a reason for hiding this comment

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

LGTM

ManagedChannel getChannel(int affinity) {
int index = affinity % channels.size();
index = Math.abs(index);
// If index is the most negative int, abs(index) is still negative.

This comment was marked as spam.

@@ -57,6 +58,11 @@
CallOptions callOptions = grpcContext.getCallOptions();
Preconditions.checkNotNull(callOptions);

return grpcContext.getChannel().newCall(descriptor, callOptions);
Channel channel = grpcContext.getChannel();
if (grpcContext.getChannelAffinity() != null && channel instanceof ChannelPool) {

This comment was marked as spam.

@pongad
Copy link
Contributor Author

pongad commented Mar 15, 2018

@vam-google Thank you for the review!

@pongad pongad merged commit 1e11861 into googleapis:master Mar 15, 2018
@pongad pongad deleted the chan-aff branch March 15, 2018 00:07
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.

4 participants