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

Adding v2.BatchingCallSettings to integrate with GCJ client#766

Merged
igorbernstein2 merged 2 commits intogoogleapis:masterfrom
rahulKQL:batchCallSettings
Aug 14, 2019
Merged

Adding v2.BatchingCallSettings to integrate with GCJ client#766
igorbernstein2 merged 2 commits intogoogleapis:masterfrom
rahulKQL:batchCallSettings

Conversation

@rahulKQL
Copy link
Copy Markdown
Contributor

@rahulKQL rahulKQL commented Jul 24, 2019

This new BatchingCallSettings enables the integration with Google-cloud-bigtable batching API.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 24, 2019
@rahulKQL rahulKQL changed the title WIP: Adding v2.BatchingCallSettings to integrate with GCJ client Adding v2.BatchingCallSettings to integrate with GCJ client Jul 24, 2019
@rahulKQL
Copy link
Copy Markdown
Contributor Author

rahulKQL commented Jul 24, 2019

cc: @igorbernstein2
@kolea2 Could you please have a look.

igorbernstein2
igorbernstein2 approved these changes Jul 24, 2019
View reviewed changes
Copy link
Copy Markdown
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 with a few nits

Comment thread gax/src/main/java/com/google/api/gax/batching/v2/BatchingCallSettings.java Outdated
Comment thread gax/src/main/java/com/google/api/gax/batching/v2/BatchingCallSettings.java Outdated
Comment thread gax/src/test/java/com/google/api/gax/batching/v2/BatchingCallSettingsTest.java Outdated
@codecov
Copy link
Copy Markdown

codecov Bot commented Jul 25, 2019

Codecov Report

Merging #766 into master will increase coverage by 0.11%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #766      +/-   ##
============================================
+ Coverage     78.14%   78.26%   +0.11%     
- Complexity     1101     1106       +5     
============================================
  Files           198      199       +1     
  Lines          4832     4858      +26     
  Branches        383      383              
============================================
+ Hits           3776     3802      +26     
  Misses          886      886              
  Partials        170      170
Impacted Files Coverage Δ Complexity Δ
...ogle/api/gax/batching/v2/BatchingCallSettings.java 100% <100%> (ø) 5 <5> (?)

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 5ad6fee...29d5f37. Read the comment docs.

@codecov
Copy link
Copy Markdown

codecov Bot commented Jul 25, 2019 •
edited
Loading

Codecov Report

Merging #766 into master will decrease coverage by 0.04%.
The diff coverage is 96%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #766      +/-   ##
============================================
- Coverage     78.19%   78.15%   -0.05%     
  Complexity     1100     1100              
============================================
  Files           197      198       +1     
  Lines          4825     4284     -541     
  Branches        380      349      -31     
============================================
- Hits           3773     3348     -425     
+ Misses          886      770     -116     
  Partials        166      166
Impacted Files Coverage Δ Complexity Δ
.../google/api/gax/batching/BatchingCallSettings.java 96% <96%> (ø) 5 <5> (?)
.../com/google/api/gax/batching/BatchingSettings.java 66.66% <0%> (-19.05%) 2% <0%> (ø)
...ava/com/google/api/gax/retrying/RetrySettings.java 15.38% <0%> (-15.87%) 2% <0%> (ø)
...m/google/api/gax/batching/FlowControlSettings.java 66.66% <0%> (-11.91%) 2% <0%> (ø)
...om/google/longrunning/stub/GrpcOperationsStub.java 77.77% <0%> (-11.88%) 10% <0%> (-1%)
...ain/java/com/google/api/gax/rpc/ClientContext.java 71.79% <0%> (-10.03%) 8% <0%> (ø)
...google/api/gax/httpjson/GaxHttpJsonProperties.java 40% <0%> (-10%) 2% <0%> (ø)
...m/google/api/gax/longrunning/OperationFutures.java 50% <0%> (-9.1%) 1% <0%> (ø)
.../grpc/GrpcServerStreamingRequestParamCallable.java 71.42% <0%> (-8.58%) 2% <0%> (ø)
...le/api/gax/core/InstantiatingExecutorProvider.java 50% <0%> (-7.15%) 3% <0%> (ø)
... and 80 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 0de16d5...620512c. Read the comment docs.

@rahulKQL
Copy link
Copy Markdown
Contributor Author

rahulKQL commented Jul 31, 2019

@igorbernstein2 I have addressed the feedback, Please have a fresh look.

igorbernstein2
igorbernstein2 suggested changes Jul 31, 2019
View reviewed changes
Comment thread gax/src/test/java/com/google/api/gax/batching/v2/BatchingCallSettingsTest.java Outdated
Comment thread gax/src/test/java/com/google/api/gax/batching/v2/BatchingCallSettingsTest.java Outdated
Comment thread gax/src/test/java/com/google/api/gax/batching/v2/BatchingCallSettingsTest.java Outdated
Comment thread gax/src/test/java/com/google/api/gax/batching/v2/BatchingCallSettingsTest.java Outdated
Comment thread gax/src/test/java/com/google/api/gax/batching/v2/BatchingCallSettingsTest.java Outdated
Comment thread gax/src/test/java/com/google/api/gax/batching/v2/BatchingCallSettingsTest.java Outdated
Comment thread gax/src/test/java/com/google/api/gax/batching/v2/BatchingCallSettingsTest.java Outdated
Comment thread gax/src/test/java/com/google/api/gax/batching/v2/BatchingCallSettingsTest.java Outdated
Comment thread gax/src/test/java/com/google/api/gax/batching/v2/BatchingCallSettingsTest.java Outdated
@igorbernstein2
Copy link
Copy Markdown
Contributor

igorbernstein2 commented Jul 31, 2019

@vam-google please take a look

@rahulKQL rahulKQL force-pushed the batchCallSettings branch from 29d5f37 to f87d2d3 Compare July 31, 2019 15:44
@rahulKQL
Copy link
Copy Markdown
Contributor Author

rahulKQL commented Jul 31, 2019

@igorbernstein2 Thanks a lot for the feedback, I have just now addressed them.

igorbernstein2
igorbernstein2 reviewed Jul 31, 2019
View reviewed changes
Comment thread gax/src/main/java/com/google/api/gax/batching/v2/BatchingCallSettings.java Outdated

/**
* A settings class to configure a {@link UnaryCallable} for calls to an API method that supports
* batching. The settings are provided using an instance of {@link BatchingSettings}.
Copy link
Copy Markdown
Contributor

@igorbernstein2 igorbernstein2 Jul 31, 2019

Choose a reason for hiding this comment

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

This shouldn't be a blocker, but it would be nice if the docs had a bit more meat.
It would be good if this explained that the retry settings are used for the batched request

Copy link
Copy Markdown
Contributor Author

@rahulKQL rahulKQL Aug 1, 2019

Choose a reason for hiding this comment

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

I have added a small explanation of retry settings usage and sample settings use. Please let me know if this looks good to you?

igorbernstein2
igorbernstein2 reviewed Jul 31, 2019
View reviewed changes
Comment thread gax/src/main/java/com/google/api/gax/batching/v2/BatchingCallSettings.java Outdated
igorbernstein2
igorbernstein2 reviewed Jul 31, 2019
View reviewed changes
Comment thread gax/src/main/java/com/google/api/gax/batching/v2/BatchingCallSettings.java Outdated
igorbernstein2
igorbernstein2 reviewed Jul 31, 2019
View reviewed changes
Comment thread gax/src/main/java/com/google/api/gax/batching/v2/BatchingCallSettings.java Outdated
igorbernstein2
igorbernstein2 approved these changes Aug 1, 2019
View reviewed changes
Copy link
Copy Markdown
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, @vam-google please take a look

Comment thread gax/src/main/java/com/google/api/gax/batching/v2/BatchingCallSettings.java Outdated
Comment thread gax/src/main/java/com/google/api/gax/batching/v2/BatchingCallSettings.java Outdated
Comment thread gax/src/main/java/com/google/api/gax/batching/v2/BatchingCallSettings.java Outdated
vam-google
vam-google approved these changes Aug 6, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@vam-google vam-google left a comment

Choose a reason for hiding this comment

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

LGTM with few minor comments/qestions

Comment thread gax/src/main/java/com/google/api/gax/batching/v2/BatchingCallSettings.java Outdated
Comment thread gax/src/test/java/com/google/api/gax/batching/v2/BatchingCallSettingsTest.java Outdated
.setRetryableCodes(retryCodes)
.setRetrySettings(retrySettings);

assertThat(builder.getRetryableCodes()).containsExactly(StatusCode.Code.UNAVAILABLE);
Copy link
Copy Markdown
Contributor

@vam-google vam-google Aug 6, 2019

Choose a reason for hiding this comment

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

Why do we need this check? Isn't checking same for the built object same thing (L85)?

Copy link
Copy Markdown
Contributor Author

@rahulKQL rahulKQL Aug 7, 2019

Choose a reason for hiding this comment

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

I had added for checking the getters of the builders. I agree that checking with the build object would be the same. So Removing this check.

Comment thread gax/src/test/java/com/google/api/gax/batching/v2/BatchingCallSettingsTest.java Outdated
} catch (Exception ex) {
actualEx = ex;
}
assertThat(actualEx).isInstanceOf(NullPointerException.class);
Copy link
Copy Markdown
Contributor

@vam-google vam-google Aug 6, 2019

Choose a reason for hiding this comment

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

The fact that BatchingCallSettings.newBuilder(null) produces NullPointerException seems logical (L115), but the fact that BatchingCallSettings.newBuilder(SQUARER_BATCHING_DESC_V2).build(); produces same - does not. Maybe IllegalArgumentException or something like that?

Copy link
Copy Markdown
Contributor

@igorbernstein2 igorbernstein2 Aug 6, 2019

Choose a reason for hiding this comment

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

good point, I would vote for IllegalStateException

Copy link
Copy Markdown
Contributor Author

@rahulKQL rahulKQL Aug 7, 2019

Choose a reason for hiding this comment

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

Thanks for pointing it out, I have updated BatchingCallSettings to now throw IllegalStateException.

igorbernstein2
igorbernstein2 suggested changes Aug 13, 2019
View reviewed changes
Copy link
Copy Markdown
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.

This needs to be rebased for the new package name from #771

@rahulKQL
Adding v2.BatchingCallSettings to integrate with GCJ client
4b7e362 
This new BatchingCallSettings enables the integration with Google-cloud-bigtable batching API.

- Added Javadoc to the v2.BatchingCallSettings
- Updated batchingSettings check to `IllegalStateException`.
- Removed repetitive checks.
- Moved back to existing batching package.
@rahulKQL
Copy link
Copy Markdown
Contributor Author

rahulKQL commented Aug 14, 2019

I have rebased this with the existing batching package. Please have a look.

igorbernstein2
igorbernstein2 reviewed Aug 14, 2019
View reviewed changes
Comment thread gax/src/main/java/com/google/api/gax/batching/BatchingCallSettings.java
Comment thread gax/src/test/java/com/google/api/gax/batching/BatchingCallSettingsTest.java Outdated
assertThat(builder.getRetryableCodes()).isEmpty();
assertThat(builder.getRetrySettings()).isNotNull();

builder.setBatchingSettings(BATCHING_SETTINGS);
Copy link
Copy Markdown
Contributor

@igorbernstein2 igorbernstein2 Aug 14, 2019

Choose a reason for hiding this comment

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

This doesn’t really belong in a test called testEmptyBuildrr

Copy link
Copy Markdown
Contributor Author

@rahulKQL rahulKQL Aug 14, 2019

Choose a reason for hiding this comment

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

Have removed builder.set## from testEmptyBuilder test case.

Comment thread gax/src/test/java/com/google/api/gax/batching/BatchingCallSettingsTest.java Outdated
@rahulKQL
Address feedback comments
620512c 
- Added javadoc for getBatchingSettings.
- Refactored junit test case.
igorbernstein2
igorbernstein2 approved these changes Aug 14, 2019
View reviewed changes
Copy link
Copy Markdown
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.
Thanks for putting this together so quickly

@igorbernstein2 igorbernstein2 merged commit 94aa7f0 into googleapis:master Aug 14, 2019
This was referenced Aug 16, 2019
Closed
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@igorbernstein2 igorbernstein2 igorbernstein2 approved these changes

@vam-google vam-google vam-google approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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.

4 participants

@rahulKQL @igorbernstein2 @vam-google @googlebot