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

Jump to bottom

feat: add experimental channel refreshing #115

Merged
merged 1 commit into from Dec 17, 2019
Merged

feat: add experimental channel refreshing #115

merged 1 commit into from Dec 17, 2019

Conversation

tonytanger
Copy link
Contributor

@tonytanger tonytanger commented Dec 12, 2019 •
edited by igorbernstein2

Implement ChannelPrimer to establish connection to the GFE so bigtable client will reconnect to the Cloud Bigtable service periodically before the connection gets terminated.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Dec 12, 2019
@tonytanger tonytanger force-pushed the add-channel-primer-reconnect-gfe branch 4 times, most recently from 8651577 to 111b544 Compare December 17, 2019 16:12
@kolea2 kolea2 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 17, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 17, 2019
@igorbernstein2
Copy link
Contributor

As discussed offline

  1. the implementation should be moved to EnhancedStubSettings
  2. the ChannelProvider meddling should be deferred to the construction of EnhancedBigtableStubSettings. Otherwise the customer has to worry about the ordering of setting the channel provider and enabling channel priming

@tonytanger tonytanger force-pushed the add-channel-primer-reconnect-gfe branch from 111b544 to 8783ce6 Compare December 17, 2019 18:33
@codecov
Copy link

codecov bot commented Dec 17, 2019 •
edited

Codecov Report

Merging #115 into master will decrease coverage by 0.06%.
The diff coverage is 65.51%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #115      +/-   ##
============================================
- Coverage     81.29%   81.23%   -0.07%     
- Complexity      937      940       +3     
============================================
  Files            95       96       +1     
  Lines          5849     5878      +29     
  Branches        325      327       +2     
============================================
+ Hits           4755     4775      +20     
- Misses          918      925       +7     
- Partials        176      178       +2
Impacted Files Coverage Δ Complexity Δ
...e/cloud/bigtable/data/v2/BigtableDataSettings.java 30% <0%> (-2.15%) 4 <0> (ø)
...ble/data/v2/stub/EnhancedBigtableStubSettings.java 94.24% <100%> (+0.49%) 18 <1> (+1) ⬆️
...loud/bigtable/data/v2/internal/RefreshChannel.java 40% <40%> (ø) 2 <2> (?)
...om/google/cloud/bigtable/emulator/v2/Emulator.java 60% <0%> (+0.86%) 14% <0%> (ø) ⬇️

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 12169d6...b9ca9cd. Read the comment docs.

@tonytanger tonytanger force-pushed the add-channel-primer-reconnect-gfe branch from 8783ce6 to 836fdda Compare December 17, 2019 18:39
@tonytanger tonytanger force-pushed the add-channel-primer-reconnect-gfe branch from 836fdda to f7aede6 Compare December 17, 2019 19:04
@kolea2 kolea2 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 17, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 17, 2019
@tonytanger tonytanger force-pushed the add-channel-primer-reconnect-gfe branch from f7aede6 to f74dbfd Compare December 17, 2019 19:52
igorbernstein2
igorbernstein2 approved these changes Dec 17, 2019
View reviewed changes
Copy link
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 landing this!

@igorbernstein2 igorbernstein2 changed the title Add ChannelPrimer to perform GFE refresh feat: add experimental channel refreshing Dec 17, 2019
kolea2
kolea2 reviewed Dec 17, 2019
View reviewed changes
...e/src/test/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStubSettingsTest.java
@@ -61,6 +61,7 @@ public void settingsAreNotLostTest() {
String projectId = "my-project";
String instanceId = "my-instance";
String appProfileId = "my-app-profile-id";
boolean isRefreshingChannel = true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you please add a small test for the default (unset) refreshingChannel as well and that it is false?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added 2 tests for default and false

@kolea2 kolea2 dismissed their stale review December 17, 2019 20:11

stale review

@kolea2 kolea2 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 17, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 17, 2019
@tonytanger tonytanger force-pushed the add-channel-primer-reconnect-gfe branch from f74dbfd to b9ca9cd Compare December 17, 2019 20:35
@kolea2 kolea2 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 17, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 17, 2019
@kolea2 kolea2 merged commit 276f942 into googleapis:master Dec 17, 2019
@igorbernstein2 igorbernstein2 mentioned this pull request Mar 9, 2020
Closed
@jc2729 jc2729 mentioned this pull request Sep 1, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@igorbernstein2 igorbernstein2 igorbernstein2 approved these changes

@kolea2 kolea2 kolea2 approved these changes

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.

None yet
5 participants
@tonytanger @igorbernstein2 @kolea2 @googlebot @kokoro-team