feat: extend channel priming logic to also send fake requests #398
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
igorbernstein2
added
the
do not merge
google-cla
bot
added
the
cla: yes
igorbernstein2
changed the title
igorbernstein2
removed
the
do not merge
product-auto-label
bot
added
the
api: bigtable
Codecov Report
@@ Coverage Diff @@
## master #398 +/- ##
============================================
+ Coverage 80.24% 80.36% +0.11%
- Complexity 1102 1113 +11
============================================
Files 105 105
Lines 6815 6897 +82
Branches 364 369 +5
============================================
+ Hits 5469 5543 +74
- Misses 1148 1155 +7
- Partials 198 199 +1
Continue to review full report at Codecov.
|
kolea2
reviewed
Aug 31, 2020
| throws IOException { | ||
| EnhancedBigtableStubSettings.Builder builder = settings.toBuilder(); | ||
|
|
||
| if (settings.isRefreshingChannel()) { |
There was a problem hiding this comment.
Can we add some tests around this to ensure the proper values are set in the settings?
There was a problem hiding this comment.
I added a test for testChannelPrimerConfigured, which should cover the behavior of it
| @@ -760,17 +787,10 @@ public EnhancedBigtableStubSettings build() { | |||
| Preconditions.checkState(projectId != null, "Project id must be set"); | |||
| Preconditions.checkState(instanceId != null, "Instance id must be set"); | |||
|
|
|||
| // Set ChannelPrimer on TransportChannelProvider so channels will gracefully refresh | |||
| // connections to Cloud Bigtable service | |||
| if (isRefreshingChannel) { | |||
| Preconditions.checkArgument( | |||
| getTransportChannelProvider() instanceof InstantiatingGrpcChannelProvider, | |||
There was a problem hiding this comment.
should this check be moved to where we are setting the transport provider, https://github.com/googleapis/java-bigtable/pull/398/files#diff-dad9668e639c962562540f7f70a24138R144?
There was a problem hiding this comment.
No, I want the validation logic to be as close to usage as possible.
...bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/BigtableChannelPrimerTest.java
Show resolved
Hide resolved
...loud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStub.java
Show resolved
Hide resolved
tonytanger
reviewed
Sep 1, 2020
...oud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/BigtableChannelPrimer.java
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
|
|
||
| private void primeChannelUnsafe(ManagedChannel managedChannel) throws IOException { |
There was a problem hiding this comment.
I'm not sure I understand why this is called unsafe?
There was a problem hiding this comment.
Because it could throw an exception, does the priming api expect to handle the exceptions?
...loud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStub.java
Show resolved
Hide resolved
| builder.setTransportChannelProvider( | ||
| transportProvider | ||
| .toBuilder() | ||
| .setChannelPrimer( |
There was a problem hiding this comment.
It would be nice to have a comment here explaining the consequence of "setChannelPrimer" that the channel is created once, but periodically creates a new channel, runs BigtableChannelPrimer on the newly created channel and replaces the existing one with the new one.
There was a problem hiding this comment.
I dont quite follow, can you rephrase?
kolea2
reviewed
Sep 1, 2020
...oud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/BigtableChannelPrimer.java
Show resolved
Hide resolved
kolea2
approved these changes
Sep 1, 2020
gcf-merge-on-green bot
pushed a commit
that referenced
this pull request
Sep 2, 2020
🤖 I have created a release \*beep\* \*boop\* --- ## [1.15.0](https://www.github.com/googleapis/java-bigtable/compare/v1.14.0...v1.15.0) (2020-09-01) ### Features * extend channel priming logic to also send fake requests ([#398](https://www.github.com/googleapis/java-bigtable/issues/398)) ([6f1ead2](https://www.github.com/googleapis/java-bigtable/commit/6f1ead2097150a87cb9712bcf35c6eaa9d57440c)) * **deps:** adopt flatten plugin and google-cloud-shared-dependencies ([#350](https://www.github.com/googleapis/java-bigtable/issues/350)) ([2298596](https://www.github.com/googleapis/java-bigtable/commit/2298596dab8a1ef87c0f48d3abe8bc3955417eb1)) ### Bug Fixes * temporarily disable reporting to unblock releases ([#395](https://www.github.com/googleapis/java-bigtable/issues/395)) ([a56a0f8](https://www.github.com/googleapis/java-bigtable/commit/a56a0f8c9caf675b68d02587b042e1feeb261ccb)) ### Dependencies * update dependency com.google.cloud:google-cloud-shared-dependencies to v0.8.6 ([#377](https://www.github.com/googleapis/java-bigtable/issues/377)) ([bdae336](https://www.github.com/googleapis/java-bigtable/commit/bdae336074d80815dcaaf8c71befafc0ed66c079)) * update dependency com.google.cloud:google-cloud-shared-dependencies to v0.9.0 ([#402](https://www.github.com/googleapis/java-bigtable/issues/402)) ([08f7d84](https://www.github.com/googleapis/java-bigtable/commit/08f7d84333c6a74bf03e0a57707b878a29400dd4)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR upgrades the existing channel refresher to send a fake read row request to ensure that server side caches are warmed up. The new refresher will create a temporary stub around the new channel, which will reuse all of the existing request decorations (resource headers and user agent)