fix(bigtable): Track number of readrows to set rowsLimit in subsequent requests #10213
Conversation
product-auto-label
bot
added
the
api: bigtable
|
@bhshkh is this still wanted/needed? |
bhshkh
changed the title
Yes |
bhshkh
changed the title
| for _, opt := range opts { | ||
| if l, ok := opt.(limitRows); ok { | ||
| rowLimitSet = true | ||
| intialRowLimit = l.limit |
There was a problem hiding this comment.
Maybe fail fast on l.limit < 0
| err = gaxInvokeWithRecorder(ctx, mt, "ReadRows", func(ctx context.Context, headerMD, trailerMD *metadata.MD, _ gax.CallSettings) error { | ||
| if rowLimitSet && numRowsRead == intialRowLimit { |
There was a problem hiding this comment.
Should we add a safety check in case numRowsRead > initialRowLimit as an extra safety measure against bugs down the stream?
| } | ||
|
|
||
| func makeReadSettings(req *btpb.ReadRowsRequest) readSettings { | ||
| return readSettings{req, nil} | ||
| func makeReadSettings(req *btpb.ReadRowsRequest, numRowsRead int64) readSettings { |
There was a problem hiding this comment.
NIT: it took me a while to understand how makeReadSettings works with readSettings.set. Maybe add a one line comment either here or on the read execution itself explaining
| @@ -965,7 +982,9 @@ func LimitRows(limit int64) ReadOption { return limitRows{limit} } | |||
|
|
|||
| type limitRows struct{ limit int64 } | |||
|
|
|||
| func (lr limitRows) set(settings *readSettings) { settings.req.RowsLimit = lr.limit } | |||
| func (lr limitRows) set(settings *readSettings) { | |||
| settings.req.RowsLimit = lr.limit - settings.numRowsRead | |||
There was a problem hiding this comment.
Maybe add a comment explaining the reasoning for this subtraction
| err = gaxInvokeWithRecorder(ctx, mt, "ReadRows", func(ctx context.Context, headerMD, trailerMD *metadata.MD, _ gax.CallSettings) error { | ||
| if rowLimitSet && numRowsRead == intialRowLimit { | ||
| return nil |
There was a problem hiding this comment.
If the number of rows requested (initialRowLimit) is higher than the total number of rows we will eventually receive, am I correct to assume that the block if err == io.EOF { would break us out of the loop?
If so, can we add a test with this scenario? (e.g. a table with 2 rows and set rowLimit = 5)
Tracking bug: b/303943537
This PR fixes TestReadRows_NoRetry_ErrorAfterLastRow test
https://github.com/googleapis/cloud-bigtable-clients-test/blob/db4d6a9e8e5e38b6f07383bd46ac14bfc8d8b9b6/tests/readrows_test.go#L171-L173
fails with error:
The test sends below request to Go Bigtable proxy:
On receiving the request, the proxy creates a ReadRows request for the client library and invokes ReadRows function.
Even though test sends RowsLimit field in the method, the proxy does not add it to the client library request. This has been fixed in this PR.