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

feat: add experimental channel refreshing #115

Merged

Conversation

tonytanger
Copy link
Contributor

@tonytanger tonytanger commented Dec 12, 2019

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 label Dec 12, 2019
@tonytanger tonytanger force-pushed the add-channel-primer-reconnect-gfe branch 4 times, most recently from 8651577 to 111b544 Compare Dec 17, 2019
@kolea2 kolea2 added the kokoro:force-run label Dec 17, 2019
@kokoro-team kokoro-team removed the kokoro:force-run label Dec 17, 2019
@igorbernstein2
Copy link
Contributor

@igorbernstein2 igorbernstein2 commented Dec 17, 2019

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 Dec 17, 2019
@codecov
Copy link

@codecov codecov bot commented Dec 17, 2019

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 Dec 17, 2019
@tonytanger tonytanger force-pushed the add-channel-primer-reconnect-gfe branch from 836fdda to f7aede6 Compare Dec 17, 2019
@kolea2 kolea2 added the kokoro:force-run label Dec 17, 2019
@kokoro-team kokoro-team removed the kokoro:force-run label Dec 17, 2019
@tonytanger tonytanger force-pushed the add-channel-primer-reconnect-gfe branch from f7aede6 to f74dbfd Compare Dec 17, 2019
Copy link
Contributor

@igorbernstein2 igorbernstein2 left a comment

LGTM!

Thanks for landing this!

@igorbernstein2 igorbernstein2 changed the title Add ChannelPrimer to perform GFE refresh feat: add experimental channel refreshing Dec 17, 2019
@@ -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

@kolea2 kolea2 Dec 17, 2019

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

@tonytanger tonytanger Dec 17, 2019

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 Dec 17, 2019

stale review

@kolea2 kolea2 added the kokoro:force-run label Dec 17, 2019
@kokoro-team kokoro-team removed the kokoro:force-run label Dec 17, 2019
@tonytanger tonytanger force-pushed the add-channel-primer-reconnect-gfe branch from f74dbfd to b9ca9cd Compare Dec 17, 2019
kolea2
kolea2 approved these changes Dec 17, 2019
@kolea2 kolea2 added the kokoro:force-run label Dec 17, 2019
@kokoro-team kokoro-team removed the kokoro:force-run label Dec 17, 2019
@kolea2 kolea2 merged commit 276f942 into googleapis:master Dec 17, 2019
10 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants