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

fix: Make watchdog @BetaApi and watchdog provider implementations @InternalApi #1119

Merged
merged 2 commits into from Jun 5, 2020

Conversation

vam-google
Copy link
Contributor

@vam-google vam-google commented Jun 4, 2020

This is to address #829.

Also add grpclb lib to bazel dependencies, as its absence was failing some of the tests well executed from bazel.

This PR basically repeats #881, with few more things in it (like making watchog final, as requested in the review there)

…ns `@InternalApi`

This is to address googleapis#829.

Also add grpclb lib to bazel dependencies, as its absence was failing some of the tests well executing from bazel.
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 4, 2020
@codecov
Copy link

codecov bot commented Jun 4, 2020

Codecov Report

Merging #1119 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1119   +/-   ##
=========================================
  Coverage     78.72%   78.72%           
  Complexity     1169     1169           
=========================================
  Files           204      204           
  Lines          5184     5184           
  Branches        416      416           
=========================================
  Hits           4081     4081           
  Misses          928      928           
  Partials        175      175           
Impacted Files Coverage Δ Complexity Δ
.../com/google/api/gax/rpc/FixedWatchdogProvider.java 100.00% <ø> (ø) 10.00 <0.00> (ø)
...gle/api/gax/rpc/InstantiatingWatchdogProvider.java 90.90% <ø> (ø) 16.00 <0.00> (ø)
...src/main/java/com/google/api/gax/rpc/Watchdog.java 80.34% <ø> (ø) 11.00 <0.00> (ø)

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 acf1fec...e37713f. Read the comment docs.

import java.util.concurrent.ScheduledExecutorService;
import javax.annotation.Nonnull;
import javax.annotation.Nullable;
import org.threeten.bp.Duration;

@BetaApi("The surface for streaming is not stable yet and may change in the future.")
public class FixedWatchdogProvider implements WatchdogProvider {
@InternalApi
Copy link
Contributor

Choose a reason for hiding this comment

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

What side effects will external users see as a result of changing this from BetaApi to InternalApi?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Strictly speaking it does not stop being beta API (the beta annotation is still there), it becomes both beta and internal. It is a conventional thing, so nothing should break because of that.

As for conventions, with both of these annotations present, people should be even more discouraged to use these classes in their code. I.e. @BetaApi may mean that this api will change in the future, and adding @InternalApi annotation indicates that the change has happened, and now the users are definitely not supposed to use this class in their code.

@igorbernstein2
Copy link
Contributor

Constructive feedback: please add some javadoc to the InternalApi tagged classes stating that they are internal and can change w/o notice.

Less constructive feedback: I still don't think that the watchdog should be exposed as public api

@vam-google vam-google changed the title fix: Make watchdog and @BetaApi and watchdog provider implementations `@InternalApi fix: Make watchdog @BetaApi and watchdog provider implementations @InternalApi Jun 5, 2020
@vam-google
Copy link
Contributor Author

@igorbernstein2, @miraleung PTAL

Copy link
Contributor

@igorbernstein2 igorbernstein2 left a comment

Choose a reason for hiding this comment

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

lgtm

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants