[gax-java] chore: generalize ExecutorService for transport layers #1098
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1098 +/- ##
============================================
+ Coverage 78.72% 78.74% +0.02%
- Complexity 1169 1191 +22
============================================
Files 204 204
Lines 5184 5213 +29
Branches 416 419 +3
============================================
+ Hits 4081 4105 +24
- Misses 928 932 +4
- Partials 175 176 +1
Continue to review full report at Codecov.
|
gax/src/main/java/com/google/api/gax/rpc/TransportChannelProvider.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you need to some explicit typecasts to avoid calling the deprecated version of withExecutor. Please see my pr for examples
Done. |
gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java
Outdated
Show resolved
Hide resolved
Note that the new method is a direct 1:1 replacement for the deprecated one, so semantically there shouldn't be coverage changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but someone from the gax team should approve
Thanks @igorbernstein2, could you please resolve the "changes requested" if no others are needed? Will wait until another gax reviewer approves. |
gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java
Show resolved
Hide resolved
gax-grpc/src/test/java/com/google/api/gax/grpc/testing/LocalChannelProvider.java
Show resolved
Hide resolved
return this; | ||
} | ||
|
||
/** @deprecated. Please use {@link #setExecutor(Executor)}. */ | ||
@Deprecated | ||
public Builder setExecutor(ScheduledExecutorService executor) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be called setExecutorProvider
and accept ExecutorProvider as an argument (i.e. put back the old method and deprecate it)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
return this; | ||
} | ||
|
||
/** @deprecated. Please use {@link #setExecutor(Executor)}. */ | ||
@Deprecated | ||
public Builder setExecutor(ScheduledExecutorService executor) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be called setExecutorProvider
and accept ExecutorProvider as an argument (i.e. put back the old method and deprecate it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
No description provided.