Watchdog is internal but uses aren't

[elharo]

Watchdog is marked internal but various places that use it aren't. e.g. gax/src/main/java/com/google/api/gax/rpc/FixedWatchdogProvider.java

@InternalApi
public class Watchdog implements Runnable 

Internal APIs should only appear on the surface of other internal APIs.

[igorbernstein2]

This is blocked on refactoring of ClientContext. I think ClientContext is supposed form the core of an autogenerated stub and contains stateful things like channels, credentials, executor, and the watchdog. This class is currently part of the public api, so unless that class becomes internal or another mechanism is introduced to store internal state, I don't really have a way forward.

[igorbernstein2]

Everything related to the watchdog is either internal or beta, so we are pretty safe. However I don't think that this is p1 bug, as I mentioned earlier, the cleanup does happen by side effect.

[elharo]

@beta != @internal and the same class can be marked as both.
This is pretty important or we're going to get locked into APIs we don't want and can't change. If we don't fix this now, we might well never be allowed to.

[igorbernstein2]

I agree that this should be fixed before we remove the Beta api labels. However since no one has expressed time pressure for removing the labels and since this doesn't have negative effects on current users, I don't think fixing this should pre-empt existing priorities.

[hkdevandla]

Hi Hanzhen, can you please take a look at this? Thanks!

[yihanzhen]

Okay this is an awkward situation here...

Ideally as what @elharo has described we should have marked Watchdog as both @BetaApi and @InternalApi, and remove @InternalApi once ClientContext starts to expose it on the surface.

How bad would it be if we change Watchdog from @InternalApi to BetaApi?

Or rather, @igorbernstein2 how is Watchdog used from ClientContext? Is it only used by BigTable? How bad would it be if we remove the methods that expose Watchdog?

[igorbernstein2]

End users should not interact with the Watchdog directly. End users are supposed to configure settings for the Watchdog via StubSettings#setStreamWatchdogCheckInterval. And the Watchdog is created internally by the client. I don't really see how you can remove the watchdog methods from ClientContext since the watchdog is needed by GrpcCallableFactory to create callable chains and there should be only 1 instance of the Watchdog per client. ClientContext encapsulates client scoped objects.

The watchdog is not Bigtable specific and is used for all server streaming grpc calls:
https://github.com/googleapis/gax-java/blob/master/gax-grpc/src/main/java/com/google/api/gax/grpc/GrpcCallableFactory.java#L304

Classes like FixedWatchdogProvider should be definitely marked Internal as well. I also think that ClientContext should also be marked Internal since it's only role is encapsulate internal client state.

[yihanzhen]

Thanks @igorbernstein2 . Can we get away with marking these classes as internal without a major bump? Or, is there any formal process for such breaking changes as making a public stable class internal (like deprecating a method or class) until we are ready for the next major version?

[igorbernstein2]

You can mark the WatchdogProvider implementations as Internal since they are BetaApi. However ClientContext is GA, so I don't think you can change that.

Also, please note that I don't have decision powers on the gax changes. I think someone from the actools team would be a better candidate for discussing the direction of the api surface. I'm mainly a consumer of the library for bigtable.

[yihanzhen]

Hey @vam-google can you weigh in?