fix: a rare race condition in the row merger (#1939) (#2002)

* fix: a rare race condition in the row merger (#1939)

* fix: a rare race condition in the row merger

This would manifest as a hang when iterating over a ServerStream from ReadRows

Change-Id: I74533c6714b40a68ec0ef81dadac747e10bee39d

* 🦉 Updates from OwlBot post-processor

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

---------

Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>

* 🦉 Updates from OwlBot post-processor

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

* chore: Fix flaky metrics tests (#1865)

This fixes a few flaky unit tests that relied on `Thread.sleep` to ensure that all metrics processing was done.  Rather than using `Thread.sleep`, we can instead use an inline event queue in the OpenCensus stats component to execute all work inline, removing the need to wait for anything to finish.

---------

Co-authored-by: Igor Bernstein <[email protected]>
Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
Co-authored-by: Steven Niemitz <[email protected]>

Author: 4 people

.github/workflows/ci.yaml

@@ -1,40 +1,54 @@
-'on':
+# Copyright 2022 Google LLC
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+# Github action job to test core java library features on
+# downstream client libraries before they are released.
+on:
   push:
     branches:
-      - 2.25.x
-  pull_request: null
+    - main
+  pull_request:
 name: ci
 jobs:
   units:
     runs-on: ubuntu-latest
     strategy:
       fail-fast: false
       matrix:
-        java:
-          - 11
-          - 17
+        java: [11, 17]
     steps:
-      - uses: actions/checkout@v3
-      - uses: actions/setup-java@v3
-        with:
-          distribution: temurin
-          java-version: ${{matrix.java}}
-      - run: java -version
-      - run: .kokoro/build.sh
-        env:
-          JOB_TYPE: test
+    - uses: actions/checkout@v3
+    - uses: actions/setup-java@v3
+      with:
+        distribution: temurin
+        java-version: ${{matrix.java}}
+    - run: java -version
+    - run: .kokoro/build.sh
+      env:
+        JOB_TYPE: test
   units-java8:
-    name: units (8)
+    # Building using Java 17 and run the tests with Java 8 runtime
+    name: "units (8)"
     runs-on: ubuntu-latest
     steps:
       - uses: actions/checkout@v3
       - uses: actions/setup-java@v3
         with:
           java-version: 8
           distribution: temurin
-      - name: >-
-          Set jvm system property environment variable for surefire plugin (unit
-          tests)
+      - name: "Set jvm system property environment variable for surefire plugin (unit tests)"
+        # Maven surefire plugin (unit tests) allows us to specify JVM to run the tests.
+        # https://maven.apache.org/surefire/maven-surefire-plugin/test-mojo.html#jvm
         run: echo "SUREFIRE_JVM_OPT=-Djvm=${JAVA_HOME}/bin/java" >> $GITHUB_ENV
         shell: bash
       - uses: actions/setup-java@v3
@@ -47,64 +61,63 @@ jobs:
   windows:
     runs-on: windows-latest
     steps:
-      - name: Support longpaths
-        run: git config --system core.longpaths true
-      - uses: actions/checkout@v3
-      - uses: actions/setup-java@v3
-        with:
-          distribution: temurin
-          java-version: 8
-      - run: java -version
-      - run: .kokoro/build.bat
-        env:
-          JOB_TYPE: test
+    - name: Support longpaths
+      run: git config --system core.longpaths true
+    - uses: actions/checkout@v3
+    - uses: actions/setup-java@v3
+      with:
+        distribution: temurin
+        java-version: 8
+    - run: java -version
+    - run: .kokoro/build.bat
+      env:
+        JOB_TYPE: test
   dependencies:
     runs-on: ubuntu-latest
     strategy:
       matrix:
-        java:
-          - 17
+        java: [17]
     steps:
-      - uses: actions/checkout@v3
-      - uses: actions/setup-java@v3
-        with:
-          distribution: temurin
-          java-version: ${{matrix.java}}
-      - run: java -version
-      - run: .kokoro/dependencies.sh
+    - uses: actions/checkout@v3
+    - uses: actions/setup-java@v3
+      with:
+        distribution: temurin
+        java-version: ${{matrix.java}}
+    - run: java -version
+    - run: .kokoro/dependencies.sh
   javadoc:
     runs-on: ubuntu-latest
     steps:
-      - uses: actions/checkout@v3
-      - uses: actions/setup-java@v3
-        with:
-          distribution: temurin
-          java-version: 17
-      - run: java -version
-      - run: .kokoro/build.sh
-        env:
-          JOB_TYPE: javadoc
+    - uses: actions/checkout@v3
+    - uses: actions/setup-java@v3
+      with:
+        distribution: temurin
+        java-version: 17
+    - run: java -version
+    - run: .kokoro/build.sh
+      env:
+        JOB_TYPE: javadoc
   lint:
     runs-on: ubuntu-latest
     steps:
-      - uses: actions/checkout@v3
-      - uses: actions/setup-java@v3
-        with:
-          distribution: temurin
-          java-version: 11
-      - run: java -version
-      - run: .kokoro/build.sh
-        env:
-          JOB_TYPE: lint
+    - uses: actions/checkout@v3
+    - uses: actions/setup-java@v3
+      with:
+        distribution: temurin
+        java-version: 11
+    - run: java -version
+    - run: .kokoro/build.sh
+      env:
+        JOB_TYPE: lint
   clirr:
     runs-on: ubuntu-latest
     steps:
-      - uses: actions/checkout@v3
-      - uses: actions/setup-java@v3
-        with:
-          distribution: temurin
-          java-version: 8
-      - run: java -version
-      - run: .kokoro/build.sh
-        env:
-          JOB_TYPE: clirr
+    - uses: actions/checkout@v3
+    - uses: actions/setup-java@v3
+      with:
+        distribution: temurin
+        java-version: 8
+    - run: java -version
+    - run: .kokoro/build.sh
+      env:
+        JOB_TYPE: clirr

README.md

@@ -50,20 +50,20 @@ If you are using Maven without the BOM, add this to your dependencies:
 If you are using Gradle 5.x or later, add this to your dependencies:
 
 ```Groovy
-implementation platform('com.google.cloud:libraries-bom:26.18.0')
+implementation platform('com.google.cloud:libraries-bom:26.26.0')
 
 implementation 'com.google.cloud:google-cloud-bigtable'
 ```
 If you are using Gradle without BOM, add this to your dependencies:
 
 ```Groovy
-implementation 'com.google.cloud:google-cloud-bigtable:2.24.1'
+implementation 'com.google.cloud:google-cloud-bigtable:2.29.1'
 ```
 
 If you are using SBT, add this to your dependencies:
 
 ```Scala
-libraryDependencies += "com.google.cloud" % "google-cloud-bigtable" % "2.24.1"
+libraryDependencies += "com.google.cloud" % "google-cloud-bigtable" % "2.29.1"
 ```
 <!-- {x-version-update-end} -->
 
@@ -609,7 +609,7 @@ Java is a registered trademark of Oracle and/or its affiliates.
 [kokoro-badge-link-5]: http://storage.googleapis.com/cloud-devrel-public/java/badges/java-bigtable/java11.html
 [stability-image]: https://img.shields.io/badge/stability-stable-green
 [maven-version-image]: https://img.shields.io/maven-central/v/com.google.cloud/google-cloud-bigtable.svg
-[maven-version-link]: https://central.sonatype.com/artifact/com.google.cloud/google-cloud-bigtable/2.24.1
+[maven-version-link]: https://central.sonatype.com/artifact/com.google.cloud/google-cloud-bigtable/2.29.1
 [authentication]: https://github.com/googleapis/google-cloud-java#authentication
 [auth-scopes]: https://developers.google.com/identity/protocols/oauth2/scopes
 [predefined-iam-roles]: https://cloud.google.com/iam/docs/understanding-roles#predefined_roles

google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/gaxx/reframing/ReframingResponseObserver.java

@@ -277,7 +277,7 @@ private void deliverUnsafe() {
       // Optimization: the inner loop will eager process any accumulated state, so reset the lock
       // for just this iteration. (If another event occurs during processing, it can increment the
       // lock to enqueue another iteration).
-      lock.lazySet(1);
+      lock.set(1);
 
       // Process the upstream message if one exists.
       pollUpstream();

google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BigtableTracerCallableTest.java

@@ -19,6 +19,7 @@
 import static org.junit.Assert.fail;
 
 import com.google.api.gax.rpc.ClientContext;
+import com.google.api.gax.rpc.ServerStream;
 import com.google.api.gax.rpc.UnavailableException;
 import com.google.bigtable.v2.BigtableGrpc.BigtableImplBase;
 import com.google.bigtable.v2.CheckAndMutateRowRequest;
@@ -54,7 +55,6 @@
 import io.grpc.Status;
 import io.grpc.StatusRuntimeException;
 import io.grpc.stub.StreamObserver;
-import io.opencensus.impl.stats.StatsComponentImpl;
 import io.opencensus.stats.StatsComponent;
 import io.opencensus.tags.TagKey;
 import io.opencensus.tags.TagValue;
@@ -74,7 +74,7 @@ public class BigtableTracerCallableTest {
 
   private FakeService fakeService = new FakeService();
 
-  private final StatsComponent localStats = new StatsComponentImpl();
+  private final StatsComponent localStats = new SimpleStatsComponent();
   private EnhancedBigtableStub stub;
   private EnhancedBigtableStub noHeaderStub;
   private int attempts;
@@ -157,10 +157,9 @@ public void tearDown() {
   }
 
   @Test
-  public void testGFELatencyMetricReadRows() throws InterruptedException {
-    stub.readRowsCallable().call(Query.create(TABLE_ID));
-
-    Thread.sleep(WAIT_FOR_METRICS_TIME_MS);
+  public void testGFELatencyMetricReadRows() {
+    ServerStream<?> call = stub.readRowsCallable().call(Query.create(TABLE_ID));
+    call.forEach(r -> {});
 
     long latency =
         StatsTestUtils.getAggregationValueAsLong(

google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracerTest.java

@@ -109,6 +109,7 @@ public class BuiltinMetricsTracerTest {
   private static final long FAKE_SERVER_TIMING = 50;
   private static final long SERVER_LATENCY = 100;
   private static final long APPLICATION_LATENCY = 200;
+  private static final long SLEEP_VARIABILITY = 15;
 
   private static final long CHANNEL_BLOCKING_LATENCY = 75;
 
@@ -353,7 +354,11 @@ public void onComplete() {
         .recordOperation(status.capture(), tableId.capture(), zone.capture(), cluster.capture());
 
     assertThat(counter.get()).isEqualTo(fakeService.getResponseCounter().get());
-    assertThat(applicationLatency.getValue()).isAtLeast(APPLICATION_LATENCY * counter.get());
+    // Thread.sleep might not sleep for the requested amount depending on the interrupt period
+    // defined by the OS.
+    // On linux this is ~1ms but on windows may be as high as 15-20ms.
+    assertThat(applicationLatency.getValue())
+        .isAtLeast((APPLICATION_LATENCY - SLEEP_VARIABILITY) * counter.get());
     assertThat(applicationLatency.getValue())
         .isAtMost(operationLatency.getValue() - SERVER_LATENCY);
   }