Repository: GoogleCloudPlatform/cloud-bigtable-client
Pull request #1780 (2018-04-04 17:55:33)

RefreshingOAuth2CredentialsInterceptor TIMEOUT is now final
The TIMEOUT existed as a way for users to override the default waiting behavior.  TIMEOUT was not final, which made it problematic as per issue #1775.  After changing TIMEOUT to 15 seconds (PR #1657), and using CallOptions deadline (#1727), users ought to have a much better experience with credentials refresh.
This PR removes threetenbp and replaces it with a long and an explicit TimeUnit.


Patch:
diff --git a/bigtable-client-core-parent/bigtable-client-core/pom.xml b/bigtable-client-core-parent/bigtable-client-core/pom.xml
index c291876a9..453a5a40b 100644
--- a/bigtable-client-core-parent/bigtable-client-core/pom.xml
+++ b/bigtable-client-core-parent/bigtable-client-core/pom.xml
@@ -107,11 +107,6 @@ limitations under the License.
             <artifactId>guava</artifactId>
             <version>${guava.version}</version>
         </dependency>
-        <dependency>
-            <groupId>org.threeten</groupId>
-            <artifactId>threetenbp</artifactId>
-            <version>${threetenbp.version}</version>
-        </dependency>
 
         <!-- Auth -->
         <dependency>
diff --git a/bigtable-client-core-parent/bigtable-client-core/src/main/java/com/google/cloud/bigtable/grpc/io/OAuthCredentialsCache.java b/bigtable-client-core-parent/bigtable-client-core/src/main/java/com/google/cloud/bigtable/grpc/io/OAuthCredentialsCache.java
index 92db299d6..4a0e91018 100644
--- a/bigtable-client-core-parent/bigtable-client-core/src/main/java/com/google/cloud/bigtable/grpc/io/OAuthCredentialsCache.java
+++ b/bigtable-client-core-parent/bigtable-client-core/src/main/java/com/google/cloud/bigtable/grpc/io/OAuthCredentialsCache.java
@@ -33,7 +33,6 @@ import java.util.concurrent.Future;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.TimeoutException;
 import javax.annotation.concurrent.GuardedBy;
-import org.threeten.bp.Duration;
 
 /**
  * This class caches calls to {@link OAuth2Credentials#refreshAccessToken()}.  It asynchronously refreshes
@@ -182,9 +181,9 @@ public class OAuthCredentialsCache {
     return headerCache;
   }
 
-  HeaderToken getHeader(Duration timeout) {
+  HeaderToken getHeader(long timeout, TimeUnit timeUnit) {
     try {
-      return getHeaderUnsafe(timeout).getToken();
+      return getHeaderUnsafe(timeout, timeUnit).getToken();
     } catch (Exception e) {
       LOG.warn("Got an unexpected exception while trying to refresh google credentials.", e);
       Status status = Status.UNAUTHENTICATED
@@ -198,7 +197,7 @@ public class OAuthCredentialsCache {
    * Get the http credential header we need from a new oauth2 AccessToken.
    */
   @VisibleForTesting
-  HeaderCacheElement getHeaderUnsafe(Duration timeout) {
+  HeaderCacheElement getHeaderUnsafe(long timeout, TimeUnit timeUnit) {
     // Optimize for the common case: do a volatile read to peek for a Good cache value
     HeaderCacheElement headerCacheUnsync = this.headerCache;
 
@@ -212,7 +211,7 @@ public class OAuthCredentialsCache {
       case Expired:
       case Exception:
         // defer the future resolution (asyncRefresh will spin up a thread that will try to acquire the lock)
-        return syncRefresh(timeout);
+        return syncRefresh(timeout, timeUnit);
       default:
         String message = "Could not process state: " + headerCacheUnsync.getCacheState();
         LOG.warn(message);
@@ -227,9 +226,9 @@ public class OAuthCredentialsCache {
    * been refreshed.
    * This method should not be called while holding the refresh lock
    */
-  private HeaderCacheElement syncRefresh(Duration timeout) {
+  private HeaderCacheElement syncRefresh(long timeout, TimeUnit timeUnit) {
     try (Closeable ss = Tracing.getTracer().spanBuilder("CredentialsRefresh").startScopedSpan()) {
-      return asyncRefresh().get(timeout.toMillis(), TimeUnit.MILLISECONDS);
+      return asyncRefresh().get(timeout, timeUnit);
     } catch (InterruptedException e) {
       LOG.warn("Interrupted while trying to refresh google credentials.", e);
       Thread.currentThread().interrupt();
diff --git a/bigtable-client-core-parent/bigtable-client-core/src/main/java/com/google/cloud/bigtable/grpc/io/RefreshingOAuth2CredentialsInterceptor.java b/bigtable-client-core-parent/bigtable-client-core/src/main/java/com/google/cloud/bigtable/grpc/io/RefreshingOAuth2CredentialsInterceptor.java
index b2b7d66b0..1af726cf5 100644
--- a/bigtable-client-core-parent/bigtable-client-core/src/main/java/com/google/cloud/bigtable/grpc/io/RefreshingOAuth2CredentialsInterceptor.java
+++ b/bigtable-client-core-parent/bigtable-client-core/src/main/java/com/google/cloud/bigtable/grpc/io/RefreshingOAuth2CredentialsInterceptor.java
@@ -33,7 +33,6 @@ import io.grpc.Status;
 
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.TimeUnit;
-import org.threeten.bp.Duration;
 
 /**
  * Client interceptor that authenticates all calls by binding header data provided by a credential.
@@ -55,7 +54,8 @@ import org.threeten.bp.Duration;
 public class RefreshingOAuth2CredentialsInterceptor implements ClientInterceptor {
   private static final Logger LOG = new Logger(RefreshingOAuth2CredentialsInterceptor.class);
 
-  public static Duration TIMEOUT = Duration.ofSeconds(15);
+  @VisibleForTesting
+  static long TIMEOUT_MILLISECONDS = TimeUnit.SECONDS.toMillis(15);
 
   @VisibleForTesting
   static final Metadata.Key<String> AUTHORIZATION_HEADER_KEY = Metadata.Key.of(
@@ -121,13 +121,13 @@ public class RefreshingOAuth2CredentialsInterceptor implements ClientInterceptor
 
       @Override
       public void start(Listener<RespT> responseListener, Metadata headers) {
-        final Duration timeout;
+        final long timeout;
         if (callOptions.getDeadline() != null) {
-          timeout = Duration.ofMillis(callOptions.getDeadline().timeRemaining(TimeUnit.MILLISECONDS));
+          timeout = callOptions.getDeadline().timeRemaining(TimeUnit.MILLISECONDS);
         } else {
-          timeout = TIMEOUT;
+          timeout = TIMEOUT_MILLISECONDS;
         }
-        HeaderToken token = store.getHeader(timeout);
+        HeaderToken token = store.getHeader(timeout, TimeUnit.MILLISECONDS);
 
         if (!token.getStatus().isOk()) {
           unauthorized = true;
diff --git a/bigtable-hbase-2.x-parent/bigtable-hbase-2.x-shaded/pom.xml b/bigtable-hbase-2.x-parent/bigtable-hbase-2.x-shaded/pom.xml
index 8846dbecd..f3e5dcb08 100644
--- a/bigtable-hbase-2.x-parent/bigtable-hbase-2.x-shaded/pom.xml
+++ b/bigtable-hbase-2.x-parent/bigtable-hbase-2.x-shaded/pom.xml
@@ -189,10 +189,6 @@ limitations under the License.
                   <pattern>org.json</pattern>
                   <shadedPattern>com.google.bigtable.repackaged.org.json</shadedPattern>
                 </relocation>
-                <relocation>
-                  <pattern>org.threeten</pattern>
-                  <shadedPattern>com.google.bigtable.repackaged.org.threeten</shadedPattern>
-                </relocation>
                 <relocation>
                   <pattern>com.lmax</pattern>
                   <shadedPattern>com.google.bigtable.repackaged.com.lmax</shadedPattern>
diff --git a/bigtable-hbase-parent/bigtable-hbase-1.x-shaded/pom.xml b/bigtable-hbase-parent/bigtable-hbase-1.x-shaded/pom.xml
index 43acefd68..048208cce 100644
--- a/bigtable-hbase-parent/bigtable-hbase-1.x-shaded/pom.xml
+++ b/bigtable-hbase-parent/bigtable-hbase-1.x-shaded/pom.xml
@@ -172,10 +172,6 @@ limitations under the License.
                   <pattern>org.json</pattern>
                   <shadedPattern>com.google.bigtable.repackaged.org.json</shadedPattern>
                 </relocation>
-                <relocation>
-                  <pattern>org.threeten</pattern>
-                  <shadedPattern>com.google.bigtable.repackaged.org.threeten</shadedPattern>
-                </relocation>
                 <relocation>
                   <pattern>com.lmax</pattern>
                   <shadedPattern>com.google.bigtable.repackaged.com.lmax</shadedPattern>
diff --git a/pom.xml b/pom.xml
index e12df5bea..42b0834b3 100644
--- a/pom.xml
+++ b/pom.xml
@@ -65,7 +65,6 @@ limitations under the License.
         <guava.version>19.0</guava.version>
         <google.auth.library.version>0.9.0</google.auth.library.version>
         <dropwizard.metrics.version>3.1.2</dropwizard.metrics.version>
-        <threetenbp.version>1.3.3</threetenbp.version>
 
 
         <beam.version>2.3.0</beam.version>


Test patch:
diff --git a/bigtable-client-core-parent/bigtable-client-core/src/test/java/com/google/cloud/bigtable/grpc/io/OAuthCredentialsCacheTest.java b/bigtable-client-core-parent/bigtable-client-core/src/test/java/com/google/cloud/bigtable/grpc/io/OAuthCredentialsCacheTest.java
index 1dc5c3ee8..0d8dd6e01 100644
--- a/bigtable-client-core-parent/bigtable-client-core/src/test/java/com/google/cloud/bigtable/grpc/io/OAuthCredentialsCacheTest.java
+++ b/bigtable-client-core-parent/bigtable-client-core/src/test/java/com/google/cloud/bigtable/grpc/io/OAuthCredentialsCacheTest.java
@@ -34,7 +34,6 @@ import org.mockito.stubbing.Answer;
 import java.io.IOException;
 import java.util.Date;
 import java.util.concurrent.*;
-import org.threeten.bp.Duration;
 
 import static org.hamcrest.CoreMatchers.containsString;
 import static org.mockito.Matchers.any;
@@ -46,7 +45,7 @@ import static org.mockito.Mockito.*;
  */
 @RunWith(JUnit4.class)
 public class OAuthCredentialsCacheTest {
-  private static Duration TIMEOUT = Duration.ofSeconds(5);
+  private static long TIMEOUT_SECONDS = 5;
 
   private static ExecutorService executorService;
 
@@ -65,21 +64,10 @@ public class OAuthCredentialsCacheTest {
   @Mock
   private OAuth2Credentials mockCredentials;
 
-  @Mock
-  private Channel mockChannel;
-
-  @Mock
-  private ClientCall mockClientCall;
-
-  @Mock
-  private ClientCall.Listener mockListener;
-
   @Before
   public void setupMocks() {
     MockitoAnnotations.initMocks(this);
     setTimeInMillieconds(0L);
-    when(mockChannel.newCall(any(MethodDescriptor.class), any(CallOptions.class)))
-        .thenReturn(mockClientCall);
   }
 
   private void setTimeInMillieconds(final long timeMs) {
@@ -103,7 +91,7 @@ public class OAuthCredentialsCacheTest {
     ExecutorService mockExecutor = Mockito.mock(ExecutorService.class);
     when(mockExecutor.submit(any(Callable.class))).thenThrow(new RuntimeException(""));
     underTest = new OAuthCredentialsCache(mockExecutor, mockCredentials);
-    Assert.assertFalse(underTest.getHeader(TIMEOUT).getStatus().isOk());
+    Assert.assertFalse(underTest.getHeader(TIMEOUT_SECONDS, TimeUnit.SECONDS).getStatus().isOk());
   }
 
   @Test
@@ -138,7 +126,8 @@ public class OAuthCredentialsCacheTest {
   public void testRefreshAfterFailure() throws Exception {
     underTest = new OAuthCredentialsCache(executorService, mockCredentials);
 
-    final AccessToken accessToken = new AccessToken("hi", new Date(HeaderCacheElement.TOKEN_STALENESS_MS + 1));
+    final AccessToken accessToken =
+        new AccessToken("hi", new Date(HeaderCacheElement.TOKEN_STALENESS_MS + 1));
 
     //noinspection unchecked
     Mockito.when(mockCredentials.refreshAccessToken())
@@ -148,11 +137,13 @@ public class OAuthCredentialsCacheTest {
         .thenReturn(accessToken);
 
     // First call
-    OAuthCredentialsCache.HeaderToken token1 = underTest.getHeader(TIMEOUT);
+    OAuthCredentialsCache.HeaderToken token1 =
+        underTest.getHeader(TIMEOUT_SECONDS, TimeUnit.SECONDS);
     Assert.assertFalse(token1.getStatus().isOk());
 
     // Now the second token should be available
-    OAuthCredentialsCache.HeaderToken token2 = underTest.getHeader(TIMEOUT);
+    OAuthCredentialsCache.HeaderToken token2 =
+        underTest.getHeader(TIMEOUT_SECONDS, TimeUnit.SECONDS);
     Assert.assertTrue(token2.getStatus().isOk());
     Assert.assertThat(token2.getHeader(), containsString("hi"));
     // Make sure that the token was only requested twice: once for the first failure & second time for background recovery
@@ -176,7 +167,8 @@ public class OAuthCredentialsCacheTest {
         .thenReturn(goodToken);
 
     // First call - setup
-    OAuthCredentialsCache.HeaderToken firstResult = underTest.getHeader(TIMEOUT);
+    OAuthCredentialsCache.HeaderToken firstResult =
+        underTest.getHeader(TIMEOUT_SECONDS, TimeUnit.SECONDS);
     Assert.assertEquals(CacheState.Good, underTest.getHeaderCache().getCacheState());
     Assert.assertThat(firstResult.getHeader(), containsString("stale"));
 
@@ -184,7 +176,7 @@ public class OAuthCredentialsCacheTest {
     setTimeInMillieconds(10);
 
     // Second call - return stale token, but schedule refresh
-    HeaderCacheElement secondResult = underTest.getHeaderUnsafe(TIMEOUT);
+    HeaderCacheElement secondResult = underTest.getHeaderUnsafe(TIMEOUT_SECONDS, TimeUnit.SECONDS);
     Assert.assertEquals(CacheState.Stale, secondResult.getCacheState());
     Assert.assertThat(secondResult.getToken().getHeader(), containsString("stale"));
 
@@ -192,7 +184,8 @@ public class OAuthCredentialsCacheTest {
     setTimeInMillieconds(expires);
 
     // Third call - now returns good token
-    OAuthCredentialsCache.HeaderToken thirdResult = underTest.getHeader(TIMEOUT);
+    OAuthCredentialsCache.HeaderToken thirdResult =
+        underTest.getHeader(TIMEOUT_SECONDS, TimeUnit.SECONDS);
     Assert.assertEquals(CacheState.Good, underTest.getHeaderCache().getCacheState());
     Assert.assertThat(thirdResult.getHeader(), containsString("good"));
 
@@ -224,13 +217,13 @@ public class OAuthCredentialsCacheTest {
     underTest.revokeUnauthToken(underTest.getHeaderCache().getToken());
     Assert.assertEquals(CacheState.Expired, underTest.getHeaderCache().getCacheState());
 
-    underTest.getHeader(TIMEOUT);
+    underTest.getHeader(TIMEOUT_SECONDS, TimeUnit.SECONDS);
     Assert.assertEquals(CacheState.Good, underTest.getHeaderCache().getCacheState());
 
     underTest.revokeUnauthToken(underTest.getHeaderCache().getToken());
     // Ensure that the revoke interceptor does not run so quickly after the previous revoke.
     // The cache state should stay good.
-    Assert.assertTrue(underTest.getHeader(TIMEOUT).getStatus().isOk());
+    Assert.assertTrue(underTest.getHeader(TIMEOUT_SECONDS, TimeUnit.SECONDS).getStatus().isOk());
   }
 
   private void testHanging()
diff --git a/bigtable-client-core-parent/bigtable-client-core/src/test/java/com/google/cloud/bigtable/grpc/io/RefreshingOAuth2CredentialsInterceptorTest.java b/bigtable-client-core-parent/bigtable-client-core/src/test/java/com/google/cloud/bigtable/grpc/io/RefreshingOAuth2CredentialsInterceptorTest.java
index 3b35aff7f..bfe9de997 100644
--- a/bigtable-client-core-parent/bigtable-client-core/src/test/java/com/google/cloud/bigtable/grpc/io/RefreshingOAuth2CredentialsInterceptorTest.java
+++ b/bigtable-client-core-parent/bigtable-client-core/src/test/java/com/google/cloud/bigtable/grpc/io/RefreshingOAuth2CredentialsInterceptorTest.java
@@ -37,10 +37,8 @@ import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
 import org.mockito.ArgumentCaptor;
-import org.mockito.Matchers;
 import org.mockito.Mock;
 import org.mockito.MockitoAnnotations;
-import org.threeten.bp.Duration;
 
 
 /**
@@ -105,7 +103,7 @@ public class RefreshingOAuth2CredentialsInterceptorTest {
     public void testDefaultDeadline() throws IOException {
         initializeOk();
         sendRequest(CallOptions.DEFAULT);
-        assertEquals(RefreshingOAuth2CredentialsInterceptor.TIMEOUT.toMillis(), getDeadlineMs());
+        assertEquals(RefreshingOAuth2CredentialsInterceptor.TIMEOUT_MILLISECONDS, getDeadlineMs());
     }
 
     @Test
@@ -129,9 +127,10 @@ public class RefreshingOAuth2CredentialsInterceptorTest {
     }
 
     private long getDeadlineMs() {
-        ArgumentCaptor<Duration> timeoutCaptor = ArgumentCaptor.forClass(Duration.class);
-        verify(mockCache, times(1)).getHeader(timeoutCaptor.capture());
-        return timeoutCaptor.getValue().toMillis();
+        ArgumentCaptor<Long> timeoutCaptor = ArgumentCaptor.forClass(Long.class);
+        ArgumentCaptor<TimeUnit> timeunitCaptor = ArgumentCaptor.forClass(TimeUnit.class);
+        verify(mockCache, times(1)).getHeader(timeoutCaptor.capture(), timeunitCaptor.capture());
+        return timeunitCaptor.getValue().toMillis(timeoutCaptor.getValue());
     }
 
     @Test
@@ -142,7 +141,7 @@ public class RefreshingOAuth2CredentialsInterceptorTest {
         Status grpcStatus = Status.UNAUTHENTICATED;
 
         // Something bad happened, and authentication could not be established
-        when(mockCache.getHeader(any(Duration.class)))
+        when(mockCache.getHeader(any(Long.class), any(TimeUnit.class)))
                 .thenReturn(new OAuthCredentialsCache.HeaderToken(grpcStatus, null));
 
         ClientCall<ReadRowsRequest, ReadRowsResponse> call = underTest.interceptCall(
@@ -219,7 +218,7 @@ public class RefreshingOAuth2CredentialsInterceptorTest {
     }
 
     private void initializeOk() {
-        when(mockCache.getHeader(any(Duration.class)))
+        when(mockCache.getHeader(any(Long.class), any(TimeUnit.class)))
                 .thenReturn(new OAuthCredentialsCache.HeaderToken(Status.OK, HEADER));
     }