Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bigtable: Add 'PartialRowsData.cancel'. #8176

Merged
merged 9 commits into from
Jun 20, 2019
Merged

Bigtable: Add 'PartialRowsData.cancel'. #8176

merged 9 commits into from
Jun 20, 2019

Conversation

mf2199
Copy link
Contributor

@mf2199 mf2199 commented May 27, 2019

Fixes #7760.

Introducing a "stop" flag into the iterable PartialRowsData() class. Setting this flag to True results in cancelling of the iteration the next time the iterator is called.

@mf2199 mf2199 requested a review from tseaver as a code owner May 27, 2019 13:27
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 27, 2019
Copy link
Contributor

@sduskis sduskis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change needs a unit test, and ideally a system test.

bigtable/google/cloud/bigtable/row_data.py Outdated Show resolved Hide resolved
bigtable/google/cloud/bigtable/row_data.py Outdated Show resolved Hide resolved
@sduskis sduskis changed the title Addressing issue #7760 Bigtable: improve reads rows cancel May 28, 2019
@sduskis
Copy link
Contributor

sduskis commented May 31, 2019

@mf2199, I had some comments that need to be addressed regarding this PR.

Appending unit-test to assert stop iteration of PartialRowsData() upon calling the cancel() method.
@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label Jun 4, 2019
@sduskis sduskis added the api: bigtable Issues related to the Bigtable API. label Jun 17, 2019
bigtable/google/cloud/bigtable/row_data.py Outdated Show resolved Hide resolved
bigtable/tests/unit/test_row_data.py Outdated Show resolved Hide resolved
@tseaver tseaver added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 19, 2019
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 19, 2019
@tseaver
Copy link
Contributor

tseaver commented Jun 19, 2019

The snippets-2.7 failures seem to be related to an outage (?) on the back-end (or maybe the Bigtable API returns 503 responses instead of using 429 for rate limiting?).

@tseaver tseaver changed the title Bigtable: improve reads rows cancel Bigtable: Add 'PartialRowsData.cancel'. Jun 19, 2019
@@ -376,6 +376,9 @@ def __init__(self, read_method, request, retry=DEFAULT_RETRY_READ_ROWS):
self.rows = {}
self._state = self.STATE_NEW_ROW

# Flag to stop iteration, for any reason not related to self.retry()
self._stop = False
Copy link
Contributor

@crwilcox crwilcox Jun 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking over the code, it seems this is marking if the iterator has been cancelled. The comment and name of this sort of directed me to think maybe this did something different. would naming this self._cancelled be accurate?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for _cancelled

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. Done.

@tseaver tseaver added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 20, 2019
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 20, 2019
@tseaver
Copy link
Contributor

tseaver commented Jun 20, 2019

@tseaver tseaver merged commit e65044c into googleapis:master Jun 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigtable Issues related to the Bigtable API. cla: yes This human has signed the Contributor License Agreement. 🚨 This issue needs some love.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ability to cancel Cloud Bigtable reads
7 participants