fix(spanner): convert enumerable GRPC::BadStatus errors to Google::Cloud::Errors

[thiagotnunes]

TLDR: we would like to have the conversion of GRPC::BadStatus errors to Google::Cloud::Errors in the Enumerable result of streaming calls of the spanner client, so that the consumer only has to deal with one kind of error.

We are currently observing a problem when calling the streaming calls in the spanner client Google::Cloud::Spanner::V1::Client where we have to handle both Google::Cloud::Errors and GRPC::Errors.

The generated client is rescuing GRPC::BadStatus and converting those to Google::Cloud::Errors (like here). That is fine, because we can rely that if we get a GRPC::BadStatus it will be automatically converted to us when executing the method.

The problem lies in handling the result of streaming calls. The generated client is returning Enumerable<PartialResultSet> (as specified here). If we call result.next we might get GRPC::BadStatus errors, because in this stream of results there is no conversion from GRPC::BadStatus errors to Google::Cloud::Errors. This makes it a bit clunky to deal with the errors, where we have to deal with both flavours of the same errors.

This problem is exemplified below:

require "google/cloud/spanner/v1"

client = V1::Spanner::Client.new

begin
  result = client.execute_streaming_sql request, opts # This can raise a Google::Cloud::Error
  result.next # This can raise a GRPC::BadStatus (or other GRPC errors)
rescue GRPC::Unavailable, Google::Cloud::UnavailableError => err # Must rescue both
  puts err
end

We have identified this problem when fixing long PDML transactions in the ruby spanner client library (googleapis/google-cloud-ruby#7592).

[thiagotnunes]

cc @dazuma

[viacheslav-rostovtsev]

So if I understand correctly, the conversion from :GRPC::BadStatus to ::Google::Cloud::Error happens for the regular requests but not for the paginated requests.
This is probably the place:

gapic-generator-ruby/gapic-common/lib/gapic/paged_enumerable.rb

Lines 128 to 133 in 1b1aaec

	next_request = @request.dup
	next_request.page_token = @page.next_page_token
	@grpc_stub.call_rpc @method_name, next_request, options: @options do |next_response, next_operation|
	@page = Page.new next_response, @resource_field, next_operation, format_resource: @format_resource
	end
	@page

[viacheslav-rostovtsev]

btw @thiagotnunes in your issue text the second 'here' hyperlink (for "The generated client is returning Enumerable as specified here") is the same as first and probably should be different?

[thiagotnunes]

@viacheslav-rostovtsev thanks for taking a look at this.

I think your understanding is correct. Thus, if we always converted from ::GRPC::BadStatus to ::Google::Cloud::Error it would simplify things in the consumers.

Thanks for pointing the wrong link. I fixed it to point to the correct block of code.