googleapis / googleapis/google-cloud-go

bigtable: return general error on mutate rows for individual row mutations

Open
#11,473 1 comment 0 reactions 1 assignee Claimed by @sushanb View on GitHub
api: bigtable type: feature request
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.