-
Notifications
You must be signed in to change notification settings - Fork 105
Set rpcTimeout to be totalTimeout for non-retried methods #712
Conversation
PTAL |
Codecov Report
@@ Coverage Diff @@
## master #712 +/- ##
============================================
+ Coverage 75.46% 75.74% +0.27%
- Complexity 1037 1041 +4
============================================
Files 196 196
Lines 4675 4679 +4
Branches 363 363
============================================
+ Hits 3528 3544 +16
+ Misses 986 975 -11
+ Partials 161 160 -1
Continue to review full report at Codecov.
|
PTAL again, changed the implementation again to what @igorbernstein2 proposed in #711 |
@@ -56,7 +56,10 @@ private Callables() {} | |||
ClientContext clientContext) { | |||
|
|||
if (areRetriesDisabled(callSettings.getRetryableCodes())) { | |||
return innerCallable; |
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 this (and below) could use a comment. Something like:
// When retries are disabled, the total timeout can be treated as the rpc timeout.
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
private static final Duration totalTimeout = Duration.ofDays(DEADLINE_IN_DAYS); | ||
|
||
@SuppressWarnings("unchecked") | ||
private static final Marshaller<String> stringMarshaller = Mockito.mock(Marshaller.class); |
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.
Consider using the MockitoRule with the @Mock
annotation here. See here for an example:
gax-java/gax/src/test/java/com/google/api/gax/tracing/TraceFinisherTest.java
Lines 49 to 50 in 33a57b6
@Rule public MockitoRule mockitoRule = MockitoJUnit.rule().strictness(Strictness.STRICT_STUBS); | |
@Mock private ApiTracer mockTracer; |
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.
Sure
Fixes #711.