Watchdog: move to client context by igorbernstein2 · Pull Request #480 · googleapis/gax-java

igorbernstein2 · 2018-02-15T22:35:18Z

This depends on #482, which handles preliminary refactoring to cut down on the noise in this PR.

This is a follow up on the watchdog introduced in #463. It moves it out of the RetryingServerStreamingCallable and into the ClientContext. The main benefits are:

Callables can be stateless again
There is only one scheduled reaper for all of the callables

Changes:

Add {Fixed,Instantiating}WatchdogProviders to allow for transitioning from built ClientContexts to StubSettings.Builders and ClientContext.Builders
Mark the Watchdog as nullable to keep backwards compatibility (to avoid NPEs building ClientContexts from Builders)
Allow StubSettings to configure both the WatchdogProvider and the WatchdogCheckInterval.
Enable the watchdog in StubSettings by default with an interval of 10 secs.
Add WatchdogServerStreamingCallable and plumb timeouts through ApiCallContext
Use the new WatchdogServerStreamingCallable and decouple RetryingServerStreamingCallable from the Watchdog

codecov-io · 2018-02-16T20:49:43Z

Codecov Report

Merging #480 into master will increase coverage by 0.1%.
The diff coverage is 68.49%.

@@ Coverage Diff @@## master #480 +/- ##
===========================================
+ Coverage 70.33% 70.44% +0.1% - Complexity 775 810 +35 
===========================================
Files 163 166 +3 Lines 3668 3776 +108 Branches 273 289 +16 ===========================================
+ Hits 2580 2660 +80 - Misses 973 993 +20 - Partials 115 123 +8

Impacted Files	Coverage Δ	Complexity Δ
...oogle/api/gax/rpc/ServerStreamingCallSettings.java	`67.27% <ø> (-4.16%)`	`6 <0> (-1)`
...e/api/gax/rpc/RetryingServerStreamingCallable.java	`0% <ø> (ø)`	`0 <0> (ø)`	⬇️
...a/com/google/api/gax/grpc/GrpcCallableFactory.java	`67.27% <0%> (-2.54%)`	`8 <0> (ø)`
...e/api/gax/rpc/WatchdogServerStreamingCallable.java	`0% <0%> (ø)`	`0 <0> (?)`
...rc/main/java/com/google/api/gax/rpc/Callables.java	`63.63% <0%> (+4.06%)`	`6 <0> (ø)`	⬇️
...m/google/api/gax/httpjson/HttpJsonCallContext.java	`29.31% <0%> (-2.18%)`	`8 <0> (ø)`
.../com/google/api/gax/rpc/FixedWatchdogProvider.java	`100% <100%> (ø)`	`9 <9> (?)`
...in/java/com/google/api/gax/rpc/ClientSettings.java	`93.24% <100%> (+1.3%)`	`12 <2> (+2)`	⬆️
...ain/java/com/google/api/gax/rpc/ClientContext.java	`82.53% <55.55%> (-11.08%)`	`9 <0> (ø)`
.../java/com/google/api/gax/grpc/GrpcCallContext.java	`73.33% <75.86%> (+0.86%)`	`30 <11> (+8)`	⬆️
... and 6 more

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 8d45d18...1e2bd88. Read the comment docs.

igorbernstein2 · 2018-02-22T22:09:21Z

rebased on the latest prep changes

igorbernstein2 · 2018-02-24T01:04:09Z

Rebased. This is ready for review

garrettjonesgoogle

The change generally looks good.

Sign in to view

+import com.google.common.base.Preconditions;
+
+/**
+ * Integration {@link ServerStreamingCallable} for the {@link Watchdog}.


Sign in to view

+
+ WatchdogProvider withExecutor(ScheduledExecutorService executor);
+
+ boolean needsExecutor();


Sign in to view

 ApiCallContext withTimeout(Duration rpcTimeout);

+ /**
+ * Returns a new ApiCallContext with the given timeout set.


Sign in to view

+ Duration getStreamWaitTimeout();
+
+ /**
+ * Returns a new ApiCallContext with the given timeout set.


Sign in to view


+ @Override
+ public ApiCallContext withStreamWaitTimeout(@Nullable Duration streamWaitTimeout) {
+ throw new UnsupportedOperationException("Http does not support streaming");


Sign in to view


+ @BetaApi("The surface for streaming is not stable yet and may change in the future.")
+ @Nullable
+ public abstract Watchdog getWatchdog();


Sign in to view

+import javax.annotation.Nullable;
+import org.threeten.bp.Duration;
+
+public class FixedWatchdogProvider implements WatchdogProvider {


igorbernstein2 · 2018-02-27T18:34:40Z

Incorporated all suggestions, PTAL

garrettjonesgoogle

LGTM - I will merge after you fix my last tiny requests

Sign in to view


 /**
- * Returns a new ApiCallContext with the given timeout set.
+ * Returns a new ApiCallContext with the given stream idel timeout set.


Sign in to view


 /**
- * Integration {@link ServerStreamingCallable} for the {@link Watchdog}.
+ * A callable that uses the {@link Watchdog} to monitor streams.


igorbernstein2 · 2018-02-27T19:13:33Z

just noticed a couple of bugs (I forgot to update the semantics to be nonnull for clientcontext). please don't merge yet

igorbernstein2 · 2018-02-28T14:03:55Z

Ok, fixed. The issue was a result of an incomplete rebase on top of the feedback in #482. The fix was to disallow null in CallContext for arguments. But to retain it internally to allow proper merge semantics. @garrettjonesgoogle PTAL

igorbernstein2 · 2018-02-28T14:07:50Z

hmm, that's odd I'm not sure why the verifyLicense check is failing

garrettjonesgoogle · 2018-02-28T14:35:52Z

We updated license headers in #487 - you'll need to update your files too.

Sign in to view

-Preconditions.checkArgument(
-streamWaitTimeout.compareTo(Duration.ZERO) > 0, "Invalid timeout: <= 0 s");
- }
+ public GrpcCallContext withStreamWaitTimeout(@Nonnull Duration streamWaitTimeout) {


- Add {Fixed,Instantiating}WatchdogProviders to allow for transitioning from built ClientContexts to StubSettings.Builders and ClientContext.Builders - Mark the Watchdog as nullable to keep backwards compatibility (to avoid NPEs building ClientContexts from Builders) - Allow StubSettings to configure both the WatchdogProvider and the WatchdogCheckInterval. - Enable the watchdog in StubSettings by default with an interval of 10 secs.

…lContext

…erStreamingCallable from the Watchdog

igorbernstein2 · 2018-02-28T15:35:31Z

updated: licenses fixed & allowing users to clear timeouts. PTAL

garrettjonesgoogle

LGTM

igorbernstein2 force-pushed the watchdog-context branch from bbd45c7 to 6aff80bCompare February 16, 2018 20:45

igorbernstein2 mentioned this pull request Feb 16, 2018
Watchdog - Refactor #482
Merged

igorbernstein2 changed the title ~~WIP: move watchdog to client context~~Watchdog to client contextFeb 16, 2018

igorbernstein2 changed the title ~~Watchdog to client context~~Watchdog: move to client contextFeb 16, 2018

igorbernstein2 force-pushed the watchdog-context branch 2 times, most recently from 0af9abf to 91256b6Compare February 22, 2018 22:09

igorbernstein2 force-pushed the watchdog-context branch from 91256b6 to 5d051cbCompare February 24, 2018 01:03

garrettjonesgoogle self-requested a review February 27, 2018 00:02

garrettjonesgoogle reviewed Feb 27, 2018
View reviewed changes

igorbernstein2 force-pushed the watchdog-context branch from 481ed3b to 93784f2Compare February 28, 2018 14:00

garrettjonesgoogle reviewed Feb 28, 2018
View reviewed changes

igorbernstein2 added 7 commits February 28, 2018 10:21

Add WatchdogServerStreamingCallable and plumb timeouts through ApiCal…
e182aa1
…lContext

Use the new WatchdogServerStreamingCallable and decouple RetryingServ…
8cbbef6
…erStreamingCallable from the Watchdog

address feedback
b6db96c

address feedback 2
c92aec0

fix nullability in call context
fe44e7c

update license
2a9c71a

igorbernstein2 force-pushed the watchdog-context branch from 2560777 to 2a9c71aCompare February 28, 2018 15:22

all users to clear stream timeouts
1e2bd88

garrettjonesgoogle approved these changes Feb 28, 2018
View reviewed changes

garrettjonesgoogle merged commit d47fccd into googleapis:masterFeb 28, 2018

igorbernstein2 deleted the watchdog-context branch March 18, 2018 06:30


		WatchdogProvider withExecutor(ScheduledExecutorService executor);

		boolean needsExecutor();

Uh oh!

Conversation

igorbernstein2 commented Feb 15, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Feb 16, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

igorbernstein2 commented Feb 22, 2018

Uh oh!

igorbernstein2 commented Feb 24, 2018

Uh oh!

garrettjonesgoogle left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

igorbernstein2 commented Feb 27, 2018

Uh oh!

garrettjonesgoogle left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

igorbernstein2 commented Feb 27, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

igorbernstein2 commented Feb 28, 2018

Uh oh!

igorbernstein2 commented Feb 28, 2018

Uh oh!

garrettjonesgoogle commented Feb 28, 2018

Uh oh!

This comment was marked as spam.

Uh oh!

igorbernstein2 commented Feb 28, 2018

Uh oh!

garrettjonesgoogle left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

igorbernstein2 commented Feb 15, 2018•
edited
Loading

codecov-io commented Feb 16, 2018•
edited
Loading

igorbernstein2 commented Feb 27, 2018•
edited
Loading