Skip to content

Bigtable: smart retry logic to cover errors in 'read_rows()' and 'read_rows(rows_limit)' requests#5966

Merged
tseaver merged 7 commits into
googleapis:masterfrom
AVaksman:bigtable-invlid_chunk-2
Sep 17, 2018
Merged

Bigtable: smart retry logic to cover errors in 'read_rows()' and 'read_rows(rows_limit)' requests#5966
tseaver merged 7 commits into
googleapis:masterfrom
AVaksman:bigtable-invlid_chunk-2

Conversation

@AVaksman
Copy link
Copy Markdown
Contributor

@AVaksman AVaksman commented Sep 13, 2018 •
edited
Loading

Attempt to fix #5876
Added logic to create a RowRange(start_key_open=last_scanned_key) if current_request.rows is empty.
This will cover table.read_rows() and table.read_rows(rows_limit) requests.

@AVaksman AVaksman requested a review from theacodes as a code owner September 13, 2018 17:42
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Sep 13, 2018
@sduskis sduskis requested review from sduskis and removed request for theacodes September 13, 2018 20:42
@sduskis
Copy link
Copy Markdown
Contributor

sduskis commented Sep 13, 2018

I spoke with @AVaksman offline. There are more changes required, but this looks like a promising lead to fix #5876

@sduskis sduskis added the api: bigtable Issues related to the Bigtable API. label Sep 13, 2018
@AVaksman AVaksman force-pushed the bigtable-invlid_chunk-2 branch from aec7df2 to f325197 Compare September 14, 2018 05:23
@AVaksman
* refactor build_updated_request() logic
6e6bb57 
* add clarifying comments
* cover erros from request with row_range where
    end_key only is set and start_key is empty (read from beginning till
    end key)
@AVaksman
add unit tests
b88cb3e
@AVaksman
split retry_request from original request
794ea2e
@AVaksman AVaksman force-pushed the bigtable-invlid_chunk-2 branch from f325197 to 794ea2e Compare September 14, 2018 05:31
@AVaksman
Copy link
Copy Markdown
Contributor Author

AVaksman commented Sep 14, 2018

@sduskis I think changes that we spoke about offline are done.
PTAL

sduskis
sduskis suggested changes Sep 14, 2018
View reviewed changes
Comment thread bigtable/google/cloud/bigtable/row_data.py Outdated
else:
new_row_ranges.append(row_range)
# if current end_key (open or closed) is set, return its value,
# if not, set to empty string ('')

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

Comment thread bigtable/google/cloud/bigtable/row_data.py
continue

# if current start_key (open or closed) is set, return its value,
# if not, then set to empty string ('')

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

Comment thread bigtable/google/cloud/bigtable/row_data.py

return new_row_ranges

def _key_already_read(self, key):

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

Comment thread bigtable/tests/unit/test_row_data.py Outdated
expected_result.rows.row_ranges.add(**self.row_range1.
expected_result_row_range = RowRange(last_scanned_key, b"row_key29",
False)
expected_result.rows.row_ranges.add(**expected_result_row_range.

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

Comment thread bigtable/tests/unit/test_row_data.py Outdated
rows_limit=8,
table_name=self.table_name)
row_range = RowRange(end_key=b"row_key29")
request.rows.row_ranges.add(**row_range.get_range_kwargs())

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

Comment thread bigtable/tests/unit/test_row_data.py Outdated
rows_limit=6)
expected_result_row_range = RowRange(last_scanned_key, b"row_key29",
False)
expected_result.rows.row_ranges.add(**expected_result_row_range.

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

Comment thread bigtable/tests/unit/test_row_data.py Outdated
rows_limit=6)
expected_result_row_range = RowRange(last_scanned_key,
start_inclusive=False)
expected_result.rows.row_ranges.add(**expected_result_row_range.

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

Comment thread bigtable/tests/unit/test_row_data.py Outdated
rows_limit=8,
table_name=self.table_name)
row_range = RowRange(start_key=b"row_key20")
request.rows.row_ranges.add(**row_range.get_range_kwargs())

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

Comment thread bigtable/tests/unit/test_row_data.py Outdated

self.assertEqual(expected_result, result)

def test_buil_updated_request_rows_limit(self):

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

Comment thread bigtable/tests/unit/test_row_data.py Outdated
get_range_kwargs())
self.assertEqual(expected_result, result)

def test_buil_updated_request_plain(self):

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@sduskis
Copy link
Copy Markdown
Contributor

sduskis commented Sep 17, 2018

@GoogleCloudPlatform/yoshi-python, can we please get a review for this? This fixes a P1 issue: #5876.

tseaver
tseaver reviewed Sep 17, 2018
View reviewed changes
Comment thread bigtable/tests/unit/test_row_data.py
expected_result = _ReadRowsRequestPB(table_name=self.table_name,
filter=row_filter.to_pb(),
rows_limit=6)
expected_result.rows.row_ranges.add(start_key_open=last_scanned_key)

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@AVaksman
rename test_build_updated_request_plain
719c232 
into test_build_updated_request_read_whole_table
@tseaver tseaver merged commit a388f46 into googleapis:master Sep 17, 2018
@zunger-humu zunger-humu mentioned this pull request Sep 17, 2018
Closed
@AVaksman AVaksman deleted the bigtable-invlid_chunk-2 branch September 18, 2018 13:59
parthea pushed a commit that referenced this pull request Nov 22, 2025
@AVaksman @tseaver
Bigtable: smart retry for 'read_rows()' and 'read_rows(rows_limit)' r…
dff4d28 
…equests (#5966)

Closes #5876.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@sduskis sduskis sduskis approved these changes

@tseaver tseaver tseaver approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

api: bigtable Issues related to the Bigtable API. cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

BigTable: google-cloud-bigtable 0.30.0 throws InvalidChunk error while iterating through a large collection

4 participants

@AVaksman @sduskis @tseaver @googlebot