ajdub508 commented on PR #28091: URL: https://github.com/apache/beam/pull/28091#issuecomment-1720973189
> Thanks for the comments documenting the test cases! was really helpful when following along. I have a few clarifying questions there and a couple requests: > > 1. Can we keep the types of exceptions we currently have? We wouldn't want to lose all that coverage. > 2. I see that the reasons added to `_NON_TRANSIENT_ERRORS` may be geared towards GRPC errors? Can we add a few reasons that BQ would return to us? for example `notFound` (this particular one is mentioned in [WriteToBigQuery ignores insert_retry_strategy on HttpErrors #21080](https://github.com/apache/beam/issues/21080)) or `accessDenied` I kept the original 3, `'invalid', 'invalidQuery', 'notImplemented'`, and have those at the top of the list. There are 2 types of error reasons in that list: 1. Reasons returned in the error list response of the `self.gcp_bq_client.insert_rows_json` call. Those are in camel case and are the ones found here in this doc - https://cloud.google.com/bigquery/docs/error-messages. 2. Reasons returned by the `GoogleAPICalError` exceptions. Those follow the HTTP standard reason format. A 404, for example will return a reason of `Not Found`, which is verified with [this integration test](https://github.com/apache/beam/pull/28091/files#diff-b6e14063dc720a5be1a68c935803f399ab5301bc3748c7f46f156b0c80f5d691R484). I can definitely add some errors from the bq docs to the list, wouldn't hurt anything, let me know if you think we should go that route. I think a lot of them probably weren't included originally because they'll actually surface a `GoogleAPICallError` exception and wouldn't be returned in the error list response of the `self.gcp_bq_client.insert_rows_json` call. When that happens, like with the 404 example, we won't see the bq camel case `notFound`, we'll end up reading the `Not Found` reason from the `GoogleAPICallError` exception that we're catching instead. That's why [this integration test](https://github.com/apache/beam/pull/28091/files#diff-b6e14063dc720a5be1a68c935803f399ab5301bc3748c7f46f156b0c80f5d691R484) works the way it does, ending up with a `Not Found` reason. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
