Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bigtable: clean up the public surface #5034

Merged
merged 7 commits into from
Apr 29, 2019

Conversation

igorbernstein2
Copy link

@igorbernstein2 igorbernstein2 commented Apr 29, 2019

  • Exclude autogenerated files that aren't used
  • Internal implementation details are now package private
  • Internal implementation details are now @internalapi
  • package-info.java documents the public surface instead of the autogen one
  • EnhancedBigtableStubSetting is now public api
  • synth.py is responsible for the transformations

* Exclude autogenerated files that aren't used
* Internal implementation details are now package private
* Internal implementation details that are needed by other pacakges are now @internalapi (ie. pagination wrappers in Client)
* package-info.java not document the public surface instead of the autogen one
* EnhancedBigtableStubSetting are now public api
* synth.py is responsible for the transformations
@igorbernstein2 igorbernstein2 requested a review from a team as a code owner April 29, 2019 15:43
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Apr 29, 2019
@igorbernstein2 igorbernstein2 added api: bigtable Issues related to the Bigtable API. and removed cla: yes This human has signed the Contributor License Agreement. labels Apr 29, 2019
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Apr 29, 2019
@igorbernstein2 igorbernstein2 added type: cleanup An internal cleanup or hygiene concern. and removed cla: yes This human has signed the Contributor License Agreement. labels Apr 29, 2019
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Apr 29, 2019
@kolea2
Copy link
Contributor

kolea2 commented Apr 29, 2019

Can/should this be done on the proto level instead?

@igorbernstein
Copy link

I don’t know of a way. Do you have a recommendation?

@igorbernstein2
Copy link
Author

Updated PR to only mark things as InternalApi instead of marking things as package private, which broke tests using Mockito to mock the stubs

Copy link
Contributor

@chingor13 chingor13 left a comment

Choose a reason for hiding this comment

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

Ideally any search/replacing we do with synthtool is temporary and can be fixed upstream in the Gapic generator. Perhaps we need another annotation/config for marking a class as internal api in the Gapic generator. Relying on finding the internal api comment is bound to fail at some point.

* </code>
* </pre>
*/
@Generated("by gapic-generator")
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we preserve this annotation so we mark the class as generated.

Copy link
Author

Choose a reason for hiding this comment

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

I was debating this myself. As far as I know nothing uses that annotation and I decided to keep it simple until its needed. If you feel strongly, I can try to retain it

@codecov
Copy link

codecov bot commented Apr 29, 2019

Codecov Report

Merging #5034 into master will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #5034      +/-   ##
============================================
+ Coverage     50.33%   50.33%   +<.01%     
+ Complexity    23676    23662      -14     
============================================
  Files          2238     2236       -2     
  Lines        226057   225941     -116     
  Branches      24961    24954       -7     
============================================
- Hits         113775   113728      -47     
+ Misses       103678   103613      -65     
+ Partials       8604     8600       -4
Impacted Files Coverage Δ Complexity Δ
...able/admin/v2/stub/GrpcBigtableTableAdminStub.java 94.48% <ø> (ø) 20 <0> (ø) ⬇️
...stub/GrpcBigtableInstanceAdminCallableFactory.java 50% <ø> (ø) 4 <0> (ø) ⬇️
...able/data/v2/stub/GrpcBigtableCallableFactory.java 37.5% <ø> (ø) 3 <0> (ø) ⬇️
...table/admin/v2/stub/BigtableInstanceAdminStub.java 3.84% <ø> (ø) 1 <0> (ø) ⬇️
...v2/stub/GrpcBigtableTableAdminCallableFactory.java 50% <ø> (ø) 4 <0> (ø) ⬇️
...ud/bigtable/data/v2/stub/BigtableStubSettings.java 82.01% <ø> (-2.16%) 14 <0> (-1)
.../cloud/bigtable/data/v2/stub/GrpcBigtableStub.java 93.79% <ø> (-0.69%) 12 <0> (-1)
...igtable/gaxx/retrying/ApiResultRetryAlgorithm.java 30.76% <ø> (ø) 6 <0> (ø) ⬇️
...ble/data/v2/stub/EnhancedBigtableStubSettings.java 98.61% <ø> (ø) 17 <0> (ø) ⬇️
...bigtable/admin/v2/stub/BigtableTableAdminStub.java 5.26% <ø> (ø) 1 <0> (ø) ⬇️
... and 9 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 ac0084f...2652323. Read the comment docs.

@igorbernstein2
Copy link
Author

As discussed offline:

Copy link
Contributor

@chingor13 chingor13 left a comment

Choose a reason for hiding this comment

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

Add a link to the feature request in the gapic generator. Otherwise LGTM.

s.copy(admin_library / 'grpc-google-cloud-bigtable-admin-v2/src', '../../google-api-grpc/grpc-google-cloud-bigtable-admin-v2/src')
s.copy(admin_library / 'proto-google-cloud-bigtable-admin-v2/src', '../../google-api-grpc/proto-google-cloud-bigtable-admin-v2/src')
# Replace javadoc and annotations before every public class with InternalApi
# javadoc and annotation
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add the link to the feature request issue for the gapic generator?

Copy link
Author

Choose a reason for hiding this comment

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

It's at the top of the file

@igorbernstein2 igorbernstein2 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 29, 2019
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 29, 2019
@igorbernstein2 igorbernstein2 merged commit 71733ab into googleapis:master Apr 29, 2019
@igorbernstein2 igorbernstein2 deleted the visibility-1 branch April 29, 2019 23:46
@JesseLovelace JesseLovelace mentioned this pull request May 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigtable Issues related to the Bigtable API. cla: yes This human has signed the Contributor License Agreement. type: cleanup An internal cleanup or hygiene concern.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants