From a6928843170416c17585c9755701f0aca9885c96 Mon Sep 17 00:00:00 2001 From: mf2199 <38331387+mf2199@users.noreply.github.com> Date: Tue, 31 Mar 2020 18:33:19 -0400 Subject: [PATCH 1/5] fix: cbt-6 [added timeout value to PartialRowsData.response_iterator] --- google/cloud/bigtable/row_data.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/google/cloud/bigtable/row_data.py b/google/cloud/bigtable/row_data.py index 24078b849..2eadf4ffa 100644 --- a/google/cloud/bigtable/row_data.py +++ b/google/cloud/bigtable/row_data.py @@ -400,7 +400,7 @@ def __init__(self, read_method, request, retry=DEFAULT_RETRY_READ_ROWS): self.read_method = read_method self.request = request self.retry = retry - self.response_iterator = read_method(request) + self.response_iterator = read_method(request, timeout=self.retry.deadline + 1) self.rows = {} self._state = self.STATE_NEW_ROW From 2f69b3819f611e4537b69fe06fc290da95b54c70 Mon Sep 17 00:00:00 2001 From: mf2199 <38331387+mf2199@users.noreply.github.com> Date: Wed, 1 Apr 2020 12:43:08 -0400 Subject: [PATCH 2/5] Reverting to using the existing Retry._deadline member --- google/cloud/bigtable/row_data.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/google/cloud/bigtable/row_data.py b/google/cloud/bigtable/row_data.py index 2eadf4ffa..a2dbb1a00 100644 --- a/google/cloud/bigtable/row_data.py +++ b/google/cloud/bigtable/row_data.py @@ -400,7 +400,7 @@ def __init__(self, read_method, request, retry=DEFAULT_RETRY_READ_ROWS): self.read_method = read_method self.request = request self.retry = retry - self.response_iterator = read_method(request, timeout=self.retry.deadline + 1) + self.response_iterator = read_method(request, timeout=self.retry._deadline + 1) self.rows = {} self._state = self.STATE_NEW_ROW From 1f938a56ad14de29292d5352899940ed77ecbc58 Mon Sep 17 00:00:00 2001 From: mf2199 <38331387+mf2199@users.noreply.github.com> Date: Mon, 1 Jun 2020 23:37:04 -0400 Subject: [PATCH 3/5] fix: added test for `retry.timeout value` --- tests/unit/test_row_data.py | 7 ++++++- tests/unit/test_table.py | 4 +++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/tests/unit/test_row_data.py b/tests/unit/test_row_data.py index b78723382..a6e363c8a 100644 --- a/tests/unit/test_row_data.py +++ b/tests/unit/test_row_data.py @@ -385,10 +385,14 @@ def test_constructor(self): self.assertEqual(partial_rows_data.retry, DEFAULT_RETRY_READ_ROWS) def test_constructor_with_retry(self): + from google.cloud.bigtable.row_data import DEFAULT_RETRY_READ_ROWS + client = _Client() client._data_stub = mock.MagicMock() - request = retry = object() + request = object() + retry = DEFAULT_RETRY_READ_ROWS partial_rows_data = self._make_one(client._data_stub.ReadRows, request, retry) + # self.assertEqual(partial_rows_data.response_iterator.timeout, 60.0) self.assertIs(partial_rows_data.request, request) self.assertEqual(partial_rows_data.rows, {}) self.assertEqual(partial_rows_data.retry, retry) @@ -471,6 +475,7 @@ def test_state_new_row_w_row(self): request = object() yrd = self._make_one(client._table_data_client.transport.read_rows, request) + self.assertEqual(yrd.retry._deadline, 60.0) yrd._response_iterator = iterator rows = [row for row in yrd] diff --git a/tests/unit/test_table.py b/tests/unit/test_table.py index d4bb621c2..964c51636 100644 --- a/tests/unit/test_table.py +++ b/tests/unit/test_table.py @@ -650,6 +650,7 @@ def test_read_rows(self): from google.cloud.bigtable import table as MUT from google.cloud.bigtable_v2.gapic import bigtable_client from google.cloud.bigtable_admin_v2.gapic import bigtable_table_admin_client + from google.cloud.bigtable.row_data import DEFAULT_RETRY_READ_ROWS data_api = bigtable_client.BigtableClient(mock.Mock()) table_api = mock.create_autospec( @@ -666,7 +667,8 @@ def test_read_rows(self): table = self._make_one(self.TABLE_ID, instance, app_profile_id=app_profile_id) # Create request_pb - request = retry = object() # Returned by our mock. + request = object() # Returned by our mock. + retry = DEFAULT_RETRY_READ_ROWS mock_created = [] def mock_create_row_request(table_name, **kwargs): From 56884c03c4645fadea662d3a3280b41e323ef92d Mon Sep 17 00:00:00 2001 From: "STATION\\mf" Date: Mon, 17 Aug 2020 19:46:52 -0400 Subject: [PATCH 4/5] test: adding relevant test following dependency release --- tests/unit/test_row_data.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/unit/test_row_data.py b/tests/unit/test_row_data.py index a6e363c8a..f56ac32af 100644 --- a/tests/unit/test_row_data.py +++ b/tests/unit/test_row_data.py @@ -392,7 +392,9 @@ def test_constructor_with_retry(self): request = object() retry = DEFAULT_RETRY_READ_ROWS partial_rows_data = self._make_one(client._data_stub.ReadRows, request, retry) - # self.assertEqual(partial_rows_data.response_iterator.timeout, 60.0) + partial_rows_data.read_method.assert_called_once_with( + request, timeout=DEFAULT_RETRY_READ_ROWS.deadline + 1 + ) self.assertIs(partial_rows_data.request, request) self.assertEqual(partial_rows_data.rows, {}) self.assertEqual(partial_rows_data.retry, retry) From 72704005c0b8aa1556488ac40dcef847e6019447 Mon Sep 17 00:00:00 2001 From: "STATION\\mf" Date: Wed, 19 Aug 2020 17:06:14 -0400 Subject: [PATCH 5/5] docs: added comment --- google/cloud/bigtable/row_data.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/google/cloud/bigtable/row_data.py b/google/cloud/bigtable/row_data.py index a2dbb1a00..7c5ede101 100644 --- a/google/cloud/bigtable/row_data.py +++ b/google/cloud/bigtable/row_data.py @@ -400,6 +400,13 @@ def __init__(self, read_method, request, retry=DEFAULT_RETRY_READ_ROWS): self.read_method = read_method self.request = request self.retry = retry + + # The `timeout` parameter must be somewhat greater than the value + # contained in `self.retry`, in order to avoid race-like condition and + # allow registering the first deadline error before invoking the retry. + # Otherwise there is a risk of entering an infinite loop that resets + # the timeout counter just before it being triggered. The increment + # by 1 second here is customary but should not be much less than that. self.response_iterator = read_method(request, timeout=self.retry._deadline + 1) self.rows = {}