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

fix(spanner): fix TransactionPingingPool throwing error ''NoneType' object is not callable' #9609

Merged
merged 6 commits into from
Nov 11, 2019
Merged

Conversation

larkee
Copy link
Contributor

@larkee larkee commented Nov 6, 2019

TransactionPingingPool is throwing error ''NoneType' object is not callable' when trying to batch insert. This is the fix.

@larkee larkee added the api: spanner Issues related to the Spanner API. label Nov 6, 2019
@larkee larkee requested a review from crwilcox November 6, 2019 09:21
@larkee larkee self-assigned this Nov 6, 2019
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Nov 6, 2019
Copy link
Contributor

@tseaver tseaver left a comment

Choose a reason for hiding this comment

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

Rather than tweak the hand-rolled _Transaction mock, let's take this as an opportunity to replace it with an "mock autospec" version, which will auttomatically keep the signatures correct.

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

larkee commented Nov 7, 2019

The mock autospec does not support static attributes which causes test_put_non_full_w_active_txn to fail because txn.committed is a NonCallableMagicMock, not None. I don't see an easy fix for that.

Perhaps, it would be easier to go with the original change and have this refactor in a subsequent PR?

@tseaver
Copy link
Contributor

tseaver commented Nov 7, 2019

@larkee Hmm, odd: I pasted in that change after making it locally, and got all the tests to pass. Nevertheless, we can update the _make_transaction() factory to assign txn.committed = None (which is an appropriate initial condition).

@larkee larkee requested a review from tseaver November 8, 2019 03:45
@larkee
Copy link
Contributor Author

larkee commented Nov 11, 2019

@tseaver PTAL

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner Issues related to the Spanner API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants