Skip to content

Conversation

@jyane
Copy link
Member

@jyane jyane commented Mar 7, 2018

No description provided.

Copy link
Contributor

@ericgribkoff ericgribkoff left a comment

Choose a reason for hiding this comment

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

Thanks for doing this! A couple comments below.

README.md Outdated

Tools
-----
APIs that are annotated with `@ExperimentalApi` are subject to change, and APIs
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we want to aim this more at library code that uses gRPC, as they may have less control of the ultimate gRPC version that gets used than an application developer directly using gRPC. Something like:

APIs that are annotated with @ExperimentalApi are subject to change, and APIs that are annotated with @Internal are for internal use by the gRPC library. Because APIs with these annotations could be modified or even removed in a future release, library code that other projects may depend on should not use these APIs. We recommend using the grpc-java-api-checker (an Error Prone plugin) to check for usages of @ExperimentalApi and @Internal in any library code that depends on gRPC.

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought all users should not use @Internal APIs. I respect your opinion :)

Copy link
Contributor

Choose a reason for hiding this comment

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

That's fair. We have the wording in @Internal's javadoc about using these APIs to implement a custom load balancer or transport, but that's probably fine to leave out here:

APIs annotated with @Internal are for internal use by the gRPC library and should not be used by gRPC users. APIs annotated with @ExperimentalApi are subject to change in future releases, and library code that other projects may depend on should not use these APIs. We recommend using the grpc-java-api-checker (an Error Prone plugin) to check for usages of @ExperimentalApi and @Internal in any library code that depends on gRPC. It may also be used to check for @Internal usage or unintended @ExperimentalApi consumption in non-library code.

Currently quite a lot of our APIs are marked experimental, so I think the "you should not use @ExperimentalApi" statement in the original text is too strong. @ejona86 WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

Oops, the load balancer reference is out-dated. Sent out #4192

Yeah, "not use" is too strong. For the library piece I think I'd say something closer to "Since libraries can not generally guarantee which version of gRPC is being used, most libraries should not use APIs annotated with @ExperimentalApi." But @ericgribkoff's most recent suggested text looks fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! I've fixed.

* <p>Note: This annotation is intended only for gRPC library code. Users should not attach this
* annotation to their own code.
* annotation to their own code. If you want to check APIs' usage, you can use
* grpc-java-api-checker.
Copy link
Contributor

Choose a reason for hiding this comment

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

How about:

 * @see <a href="https://github.com/grpc/grpc/grpc-java-api-checker">grpc-java-api-checker</a>, an 
 * Error Prone plugin to automatically check for usages of this API.

(Same for Internal.java)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@jyane jyane force-pushed the add-api-checker branch 2 times, most recently from 99acd0d to a8ff84c Compare March 10, 2018 02:50
@jyane jyane force-pushed the add-api-checker branch from a8ff84c to 0b9ae12 Compare March 10, 2018 02:51
@jyane
Copy link
Member Author

jyane commented Mar 10, 2018

@ericgribkoff PTAL

@jyane
Copy link
Member Author

jyane commented Mar 10, 2018

Wait... checkstyles are failed... I will fix it.

@jyane
Copy link
Member Author

jyane commented Mar 10, 2018

I've fixed but I've found new warnings on :javadoc. (d76746a)

.:grpc-core:javadocPicked up _JAVA_OPTIONS: -Xmx2048m -Xms512m
/home/travis/build/grpc/grpc-java/core/src/main/java/io/grpc/ExperimentalApi.java:52: warning - Tag @see: missing final '>': "<a href="https://github.com/grpc/grpc-java-api-checker">grpc-java-api-checker</a>, an Error
     Prone plugin to automatically check for usages of this API."
/home/travis/build/grpc/grpc-java/core/src/main/java/io/grpc/Internal.java:48: warning - Tag @see: missing final '>': "<a href="https://github.com/grpc/grpc-java-api-checker">grpc-java-api-checker</a>, an Error
     Prone plugin to automatically check for usages of this API."

2 warnings

and It could be reproduced on my local machine.

$ ./gradlew :grpc-core:javadoc

> Configure project :grpc-compiler
*** Building codegen requires Protobuf version 3.5.1-1
*** Please refer to https://github.com/grpc/grpc-java/blob/master/COMPILING.md#how-to-build-code-generation-plugin

> Task :grpc-core:javadoc
/Users/jyane/workspace/repos/github.com/jyane/grpc-java/core/src/main/java/io/grpc/ExperimentalApi.java:52: warning - Tag @see: missing final '>': "<a href="https://github.com/grpc/grpc-java-api-checker">grpc-java-api-checker</a>, an Error
     Prone plugin to automatically check for usages of this API."
/Users/jyane/workspace/repos/github.com/jyane/grpc-java/core/src/main/java/io/grpc/Internal.java:49: warning - Tag @see: missing final '>': "<a href="https://github.com/grpc/grpc-java-api-checker">grpc-java-api-checker</a>, an Error
      Prone plugin to automatically check for usages of this API."
2 warnings


BUILD SUCCESSFUL in 4s
6 actionable tasks: 1 executed, 5 up-to-date

Using a link with strings might not be permitted.
https://docs.oracle.com/javase/8/docs/technotes/tools/windows/javadoc.html#CHDDIEDI

Thus, I've removed @see. @ericgribkoff What do you think? I might be something wrong.

Copy link
Contributor

@ericgribkoff ericgribkoff left a comment

Choose a reason for hiding this comment

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

LGTM

@jyane
Copy link
Member Author

jyane commented Mar 28, 2018

I want to merge this PR once grpc-java-api-checker:1.1.0 was released.

ref: grpc/grpc-java-api-checker#15

@ericgribkoff ericgribkoff merged commit 5af2515 into grpc:master Mar 28, 2018
@jyane jyane deleted the add-api-checker branch March 29, 2018 00:38
@jyane
Copy link
Member Author

jyane commented Mar 29, 2018

Thanks!

@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants