Skip to content

storage: add benchwrapper #6062

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.

Sign up for GitHub

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

Jump to bottom
Merged
chingor13 merged 1 commit into from
Sep 6, 2019
Merged

storage: add benchwrapper #6062

chingor13 merged 1 commit into from
Sep 6, 2019

Conversation

@jeanbza
Copy link

@jeanbza jeanbza commented Aug 11, 2019 •
edited
Loading

This adds a small gRPC server around the google-cloud-storage client library,
which allows g3 benchmarking code to prod at the library and record
measurements without needing to be written in Java / be in the same repository.

Since g3 requires TLS on all connections, the server is TLS-ed with a dummy
cert. These certs aren't intended to be private: the kokoro environment
is the only place this gets run. (never in prod, etc)

ps: I'm not sure what the right place to put this code is. Minimally it requires the local version of google-cloud-storage, so I stuck it alongside it. Perhaps there's a better place, with a way to access google-cloud-storage without going to maven though.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Aug 11, 2019
@chingor13
Copy link
Contributor

chingor13 commented Aug 13, 2019

I don't think this is the right place for this code - it will be shipped with the client library and adds additional dependencies for the consumers of google-cloud-java. It probably belongs in its own artifact (probably under google-cloud-testing/) which can depend upon the snapshot version of google-cloud-storage. In a test environment, we install all artifacts to the local maven repository and so maven will find the snapshot version before trying to fetch from Maven Central.

@codecov
Copy link

codecov bot commented Aug 14, 2019 •
edited
Loading

Codecov Report

Merging #6062 into master will decrease coverage by 0.36%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #6062      +/-   ##
============================================
- Coverage     47.38%   47.02%   -0.37%     
+ Complexity    27182    25717    -1465     
============================================
  Files          2523     2523              
  Lines        274600   269325    -5275     
  Branches      31383    31190     -193     
============================================
- Hits         130124   126647    -3477     
+ Misses       134863   134000     -863     
+ Partials       9613     8678     -935
Impacted Files Coverage Δ Complexity Δ
...m/google/cloud/vision/v1/ImageAnnotatorClient.java 58.69% <0%> (-3.31%) 15% <0%> (-4%)
...e/cloud/vision/v1p4beta1/ImageAnnotatorClient.java 58.69% <0%> (-3.31%) 15% <0%> (-4%)
...e/cloud/vision/v1p3beta1/ImageAnnotatorClient.java 47.05% <0%> (-2.95%) 9% <0%> (-2%)
...e/cloud/vision/v1p2beta1/ImageAnnotatorClient.java 47.05% <0%> (-2.95%) 9% <0%> (-2%)
.../java/com/google/cloud/speech/v1/SpeechClient.java 48.57% <0%> (-2.78%) 10% <0%> (-2%)
...om/google/cloud/speech/v1p1beta1/SpeechClient.java 48.57% <0%> (-2.78%) 10% <0%> (-2%)
...d/webrisk/v1beta1/WebRiskServiceV1Beta1Client.java 63.41% <0%> (-2.5%) 12% <0%> (-3%)
...ogle/cloud/texttospeech/v1/TextToSpeechClient.java 55.88% <0%> (-2.46%) 9% <0%> (-2%)
...cloud/texttospeech/v1beta1/TextToSpeechClient.java 55.88% <0%> (-2.46%) 9% <0%> (-2%)
...oogle/cloud/language/v1/LanguageServiceClient.java 79.16% <0%> (-2.32%) 26% <0%> (-9%)
... and 637 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 3909ebd...db88471. Read the comment docs.

@jeanbza
Copy link
Author

jeanbza commented Aug 14, 2019

I don't think this is the right place for this code - it will be shipped with the client library and adds additional dependencies for the consumers of google-cloud-java. It probably belongs in its own artifact (probably under google-cloud-testing/)

Done

In a test environment, we install all artifacts to the local maven repository and so maven will find the snapshot version before trying to fetch from Maven Central.

What are the commands that need to be run to do that?

kolea2
kolea2 reviewed Aug 19, 2019
View reviewed changes
chingor13
chingor13 requested changes Aug 19, 2019
View reviewed changes
@jeanbza
Copy link
Author

jeanbza commented Aug 19, 2019 •
edited
Loading

Thanks for the second review. All comments addressed.

kolea2
kolea2 previously requested changes Aug 20, 2019
View reviewed changes
@jeanbza
Copy link
Author

jeanbza commented Aug 20, 2019

Thanks for the third review. All comments addressed.

elharo
elharo reviewed Aug 20, 2019
View reviewed changes
@jeanbza
Copy link
Author

jeanbza commented Aug 20, 2019

Thanks for the fourth review. All comments addressed.

elharo
elharo reviewed Aug 20, 2019
View reviewed changes
BenWhitehead
BenWhitehead reviewed Aug 20, 2019
View reviewed changes
elharo
elharo reviewed Aug 20, 2019
View reviewed changes
@jeanbza
Copy link
Author

jeanbza commented Aug 21, 2019

Thanks for the fifth review. All comments addressed.

elharo
elharo reviewed Aug 21, 2019
View reviewed changes
@jeanbza
storage: add benchwrapper
db88471 
This adds a small gRPC server around the google-cloud-storage client library,
which allows g3 benchmarking code to prod at the library and record
measurements without needing to be written in Java / be in the same repository.

Since g3 requires TLS on all connections, the server is TLS-ed with a dummy
cert. These certs aren't intended to be private: the kokoro environment
is the only place this gets run. (never in prod, etc)
@jeanbza
Copy link
Author

jeanbza commented Aug 21, 2019

Thanks for the sixth review. All comments addressed.

@kolea2 kolea2 dismissed their stale review August 22, 2019 02:23

comments addressed

@chingor13 chingor13 merged commit 569e485 into googleapis:master Sep 6, 2019
@kolea2 kolea2 mentioned this pull request Sep 11, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chingor13 chingor13 chingor13 approved these changes

@JesseLovelace JesseLovelace Awaiting requested review from JesseLovelace

+3 more reviewers

@elharo elharo elharo left review comments

@BenWhitehead BenWhitehead BenWhitehead left review comments

@kolea2 kolea2 kolea2 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@JesseLovelace JesseLovelace

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@jeanbza @chingor13 @elharo @BenWhitehead @kolea2 @googlebot @JesseLovelace