googleapis / googleapis/google-cloud-go
bigtable: return general error on mutate rows for individual row mutations
- Dominant language
- Go
- Stars
- 4.5k
- Forks
- 1.6k
- Avg merge
- 1d 13h
- Merged PRs (30d)
- 109
Description
## Is your feature request related to a problem? Please describe.
The current ApplyBulk [implementation](https://github.com/googleapis/google-cloud-go/blob/bigtable/v1.34.0/bigtable/bigtable.go#L1236-L1239) returns `(nil, err)` in case of a failure in the entire operation, and `([]err, nil)` in case of individual mutation failures. This is inconsistent with other client implementations (e.g. [python](https://github.com/googleapis/python-bigtable/blob/v2.28.1/google/cloud/bigtable/batcher.py#L414), [java (on gax)](https://github.com/googleapis/sdk-platform-java/blob/v2.51.1/gax-java/gax/src/main/java/com/google/api/gax/batching/BatcherImpl.java#L351)), and is also more error prone (2 different fields to check for errors vs a single source of truth).
## Describe the solution you'd like
Create a new `ApplyBulkV2` (or whatever the name is), use a return value of `err error` and aggregate individual mutations exceptions in some aggregated error type. Deprecate the existing method.
## Describe alternatives you've considered
- Changing the existing method: not viable since that would be a breaking change.
- Keep returning `(errs []error, err error)` but always set the second parameter if there are any individual exceptions: seems redundant when we can aggregate the exceptions on a meaningful type.
## Additional context
The way the conformance tests [Test Framework](https://github.com/googleapis/cloud-bigtable-clients-test) is setup needs us to implement a proxy server to proxy requests for each client's implementation. We would need then to change the [method](https://github.com/googleapis/google-cloud-go/blob/bigtable/v1.34.0/bigtable/internal/testproxy/proxy.go#L698) we currently call on our proxy server, potentially leaving the deprecated implementation not covered by conformance tests.
To make this safer we should make sure both the new and the deprecated `ApplyBulk` go through the same code path, and change only how each of them returns errors.
Contributor guide
Assessment
This issue has not been assessed yet.