Skip to content

Remove invalid chunk validation - #2992

Merged
tseaver merged 4 commits into
googleapis:masterfrom
garye:patch-1
Feb 14, 2017
Merged

Remove invalid chunk validation#2992
tseaver merged 4 commits into
googleapis:masterfrom
garye:patch-1

Conversation

@garye

@garyegarye commented Feb 9, 2017

Copy link
Copy Markdown
Contributor

Fixes#2980

@googlebotgooglebot added the cla: yes This human has signed the Contributor License Agreement. label Feb 9, 2017
@lukesneeringer

Copy link
Copy Markdown
Contributor

@dhermes Can you glance at this one? I lack sufficient context.

@dhermes

Copy link
Copy Markdown
Contributor

@garye I'd guess there would be broken tests, but it's hard to say right now because there is a bad release of protobuf that is breaking everything

assert self.state == self.ROW_IN_PROGRESS
self._validate_chunk_status(chunk)
if not chunk.HasField('commit_row') and not chunk.reset_row:
_raise_if(not chunk.timestamp_micros or not chunk.value)

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@lukesneeringer

Copy link
Copy Markdown
Contributor

@dhermes Update on the protobuf thing: googleapis/api-client-staging#159

@dhermes

Copy link
Copy Markdown
Contributor

I restarted the CircleCI build: https://circleci.com/gh/GoogleCloudPlatform/google-cloud-python/865

Hopefully it works?

@garye

Copy link
Copy Markdown
ContributorAuthor

Can someone help me finish this off? The test failures don't really seem related. Getting this fix pushed and released will unblock a number of users.

@dhermes

Copy link
Copy Markdown
Contributor

@garye Just re-started the CircleCI build: https://circleci.com/gh/GoogleCloudPlatform/google-cloud-python/876

Hopefully the bad imports are fully resolved. I'll follow-up if not.

@dhermes

Copy link
Copy Markdown
Contributor

@garye So we've got a unit test failing that is actually caused by this PR:


def test_invalid_empty_second_chunk(self):
from google.cloud.bigtable.row_data import InvalidChunk
chunks = _generate_cell_chunks(['', ''])
first = chunks[0]
first.row_key = b'RK'
first.family_name.value = 'A'
first.qualifier.value = b'C'
response = _ReadRowsResponseV2(chunks)
iterator = _MockCancellableIterator(response)
prd = self._make_one(iterator)
with self.assertRaises(InvalidChunk):
> prd.consume_next()
E AssertionError: InvalidChunk not raised

ISTM the right move is just to delete this test case since it no longer applies?

@garye

Copy link
Copy Markdown
ContributorAuthor

yeah, i think that's an invalid test case

@dhermes

Copy link
Copy Markdown
Contributor

Ping me when this PR is updated with the test case removed? You can ignore the speech test failures. It looks like the underlying protobuf libraries are (now correctly) using single instead of double precision for message fields with single precision

@garye

garye commented Feb 13, 2017 via email

Copy link
Copy Markdown
ContributorAuthor

with self.assertRaises(InvalidChunk):
prd.consume_next()


This comment was marked as spam.

This comment was marked as spam.

@garye

Copy link
Copy Markdown
ContributorAuthor

Yeah, thanks I noticed that and (hopefully) pushed a fix.

@tseaver

Copy link
Copy Markdown
Contributor

@garye Ugh, you need to be able to run tox -e lint locally:

bigtable/unit_tests/test_row_data.py:419:1: W293blanklinecontainswhitespace

@garye

garye commented Feb 13, 2017 via email

Copy link
Copy Markdown
ContributorAuthor

@tseaver

Copy link
Copy Markdown
Contributor

I think it should just work to run tox -e pylint. You probably want to export the following environment variables so it only runs on your changes:

$ export GOOGLE_CLOUD_TESTING_REMOTE=upstream # the name of the remote for this repo
$ export GOOGLE_CLOUD_TESTING_BRANCH=master
$ tox -e lint

@dhermes

Copy link
Copy Markdown
Contributor

LGTM

@tseaver
tseaver merged commit 951f5d0 into googleapis:masterFeb 14, 2017
@garye
garye deleted the patch-1 branch February 14, 2017 23:01
@garye

Copy link
Copy Markdown
ContributorAuthor

Thanks guys!

@dhermes

Copy link
Copy Markdown
Contributor

@garye We hope to cut a release in the next 2-3 days as well

richkadel pushed a commit to richkadel/google-cloud-python that referenced this pull request May 6, 2017
parthea pushed a commit that referenced this pull request Nov 22, 2025
Remove invalid chunk validation
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yesThis human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@garye@lukesneeringer@dhermes@tseaver@googlebot