Skip to content

Commit dc66f9d

Browse files
fix: prevent scanner cancellation from reading an extra row (#3759)
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly: - [ ] Make sure to open an issue as a [bug/issue](https://github.com/googleapis/java-bigtable-hbase/issues/new/choose) before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea - [ ] Ensure the tests and linter pass - [ ] Code coverage does not decrease (if any source code was changed) - [ ] Appropriate docs were updated (if necessary) Fixes #<issue_number_goes_here> ☕️ If you write sample code, please follow the [samples format]( https://github.com/GoogleCloudPlatform/java-docs-samples/blob/main/SAMPLE_FORMAT.md).
1 parent f40496c commit dc66f9d

File tree

3 files changed

+31
-6
lines changed

3 files changed

+31
-6
lines changed

bigtable-client-core-parent/bigtable-hbase/src/main/java/com/google/cloud/bigtable/hbase/wrappers/veneer/DataClientVeneerApi.java

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -254,9 +254,7 @@ public Result next() {
254254

255255
@Override
256256
public void close() {
257-
if (iterator.hasNext()) {
258-
serverStream.cancel();
259-
}
257+
serverStream.cancel();
260258
}
261259

262260
public boolean renewLease() {

bigtable-client-core-parent/bigtable-hbase/src/test/java/com/google/cloud/bigtable/hbase/wrappers/veneer/TestDataClientVeneerApi.java

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@
4848
import com.google.protobuf.ByteString;
4949
import io.grpc.stub.StreamObserver;
5050
import java.io.IOException;
51+
import java.util.Iterator;
5152
import java.util.List;
5253
import org.apache.hadoop.hbase.Cell;
5354
import org.apache.hadoop.hbase.client.Result;
@@ -224,6 +225,34 @@ public void testReadRows() throws IOException {
224225
.call(Mockito.eq(query), Mockito.any(GrpcCallContext.class));
225226
}
226227

228+
@Test
229+
public void testReadRowsCancel() throws IOException {
230+
231+
Query query = Query.create(TABLE_ID).rowKey(ROW_KEY);
232+
when(mockDataClient.readRowsCallable(Mockito.<RowResultAdapter>any()))
233+
.thenReturn(mockStreamingCallable)
234+
.thenReturn(mockStreamingCallable);
235+
236+
when(mockStreamingCallable.call(Mockito.eq(query), Mockito.any(GrpcCallContext.class)))
237+
.thenReturn(serverStream);
238+
239+
Iterator<Result> mockIter = Mockito.mock(Iterator.class);
240+
when(serverStream.iterator()).thenReturn(mockIter);
241+
when(mockIter.hasNext()).thenReturn(true);
242+
when(mockIter.next()).thenReturn(EXPECTED_RESULT);
243+
244+
ResultScanner resultScanner = dataClientWrapper.readRows(query);
245+
assertResult(EXPECTED_RESULT, resultScanner.next());
246+
247+
doNothing().when(serverStream).cancel();
248+
resultScanner.close();
249+
250+
// make sure that the scanner doesn't iteract with the iterator on close
251+
verify(serverStream).cancel();
252+
verify(mockIter, times(1)).hasNext();
253+
verify(mockIter, times(1)).next();
254+
}
255+
227256
@Test
228257
public void testReadRowsAsync() throws Exception {
229258
Query query = Query.create(TABLE_ID).rowKey(ROW_KEY);

bigtable-hbase-1.x-parent/bigtable-hbase-1.x/src/test/java/com/google/cloud/bigtable/hbase1_x/TestMetrics.java

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -271,9 +271,7 @@ public void testFirstResponseLatency() throws IOException {
271271
.get("google-cloud-bigtable.grpc.method.ReadRows.firstResponse.latency")
272272
.get();
273273

274-
// adding buffer time to the upper range to allow for a race between the emulator and the client
275-
// recording the duration
276-
assertThat(firstResponseLatencyMetric).isAtMost(methodInvocationLatency - 20 / 2);
274+
assertThat(firstResponseLatencyMetric).isAtMost(methodInvocationLatency);
277275
}
278276

279277
@Test

0 commit comments

Comments
 (0)