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: Add response object to any append rows requests exception #838

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

film42
Copy link

@film42 film42 commented Oct 11, 2024

Should an AppendRowsRequest fail, you need to inspect the response to see what went wrong. Currently this lib only raises an exception with the code and message, throwing the actual response away. This patch adds the response to any exception raise. This is fine because the base grpc error has a response kwarg that this lib wasn't using.

Now you can catch the error and call e.response.row_errors to see the underlying row errors.

Fixes: #836

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #836 🦕

NOTE: I didn't have time to set up the system test env to verify the test I added passes so I'm just going to test it in CI here on github. If it doesn't work we can probably just delete it.

Should an AppendRowsRequest fail, you need to inspect the response to
see what went wrong. Currently this lib only raises an exception with
the code and message, throwing the actual response away. This patch adds
the response to any exception raise. This is fine because the base grpc
error has a response kwarg that this lib wasn't using.

Now you can catch the error and call `e.response.row_errors` to see the
underlying row errors.

Fixes: googleapis#836
@film42 film42 requested review from a team as code owners October 11, 2024 17:32
Copy link

google-cla bot commented Oct 11, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@product-auto-label product-auto-label bot added the size: xs Pull request size is extra small. label Oct 11, 2024
@product-auto-label product-auto-label bot added the api: bigquerystorage Issues related to the googleapis/python-bigquery-storage API. label Oct 11, 2024
@yirutang yirutang enabled auto-merge (squash) October 16, 2024 16:58
@yirutang yirutang added the kokoro:run Add this label to force Kokoro to re-run the tests. label Oct 16, 2024
@yoshi-kokoro yoshi-kokoro removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Oct 16, 2024
@yirutang yirutang added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 18, 2024
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 18, 2024
@bhavitsharma
Copy link

bhavitsharma commented Oct 23, 2024

any way to prioritize this PR? it's very very troublesome to debug any errors with write APIs without modifying the source code. @yirutang @agrawal-siddharth

@yirutang
Copy link
Contributor

any way to prioritize this PR? it's very very troublesome to debug any errors with write APIs without modifying the source code. @yirutang @agrawal-siddharth

Sorry about this! The release process is experiencing some blockers and I am looping in DevRel experts to answer this question. @leahecole

@leahecole leahecole added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 24, 2024
@yoshi-kokoro yoshi-kokoro removed kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Oct 24, 2024
@leahecole
Copy link
Contributor

Hi @yirutang and @bhavitsharma - this PR is still on our radar - as Yiru said, the entire release process has an issue right now that we all are eagerly anticipating being resolved. The fix is being rolled out incrementally and should be in this repo soon. I am making sure that @yirutang is cc'd on the appropriate bug for updates and I do check on this process every day and reach out to the engineers working on the fix.

@bhavitsharma
Copy link

Thank you for the update!

@davidcavazos davidcavazos added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 24, 2024
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 24, 2024
Copy link
Contributor

@parthea parthea left a comment

Choose a reason for hiding this comment

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

Please see the following failure which appears when using pre-release version of protobuf

=================================== FAILURES ===================================
_____________ test_append_rows_with_invalid_stream_name_fails_fast _____________

bqstorage_write_client = 

    def test_append_rows_with_invalid_stream_name_fails_fast(bqstorage_write_client):
        bad_request = gapic_types.AppendRowsRequest()
        bad_request.write_stream = "this-is-an-invalid-stream-resource-path"
    
        with pytest.raises(exceptions.GoogleAPICallError) as excinfo:
            bqstorage_write_client.append_rows(bad_request)
    
>       assert isinstance(excinfo.value.response, gapic_types.AppendRowsResponse)
E       assert False
E        +  where False = isinstance(<_MultiThreadedRendezvous of RPC that terminated with:\n	status = StatusCode.UNKNOWN\n	details = "Exception iterating requests!"\n	debug_error_string = "None"\n>, )
E        +    where <_MultiThreadedRendezvous of RPC that terminated with:\n	status = StatusCode.UNKNOWN\n	details = "Exception iterating requests!"\n	debug_error_string = "None"\n> = Unknown('Exception iterating requests!').response
E        +      where Unknown('Exception iterating requests!') = .value
E        +    and    = gapic_types.AppendRowsResponse
nox > python -c 'import google.protobuf; print(google.protobuf.__version__)'
5.29.0rc2

@parthea parthea added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Oct 25, 2024
@parthea parthea disabled auto-merge October 25, 2024 17:17
@parthea
Copy link
Contributor

parthea commented Oct 25, 2024

Adding do not merge until Kokoro Prerelease Dependencies is fixed. See #838 (review)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigquerystorage Issues related to the googleapis/python-bigquery-storage API. do not merge Indicates a pull request not ready for merge, due to either quality or timing. size: xs Pull request size is extra small.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No way to observe the AppendRowsResponse row_errors
8 participants