Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ReadRows merge acceptance tests #8

Closed
igorbernstein2 opened this issue Feb 4, 2019 · 2 comments · Fixed by #10
Closed

ReadRows merge acceptance tests #8

igorbernstein2 opened this issue Feb 4, 2019 · 2 comments · Fixed by #10
Assignees
Labels
bug Something isn't working enhancement New feature or request
Milestone

Comments

@igorbernstein2
Copy link

Is your feature request related to a problem? Please describe.
Hi,
I've been working on a new java client for bigtable and ran across this repo. I would like to thank you creating this client. I think it's a great addition to the Bigtable ecosystem. While poking around I noticed that there some edge cases that weren't covered in this client's ReadRows implementation. For example, it doesn't appear like the reset flag is respected.

Describe the solution you'd like
On the Cloud Bigtable team we created a json file to help test ReadRow chunk parsing and I wanted to let you know of its existence:
https://github.com/googleapis/google-cloud-java/blob/master/google-cloud-clients/google-cloud-bigtable/src/test/resources/com/google/cloud/bigtable/data/v2/stub/readrows/read-rows-acceptance-test.json
Each test case has a list of chunks and the expected Row results.

Additional context
Here are a couple of links of other clients using the file:

Thanks again for publishing this client!

@jscott22
Copy link
Contributor

jscott22 commented Feb 5, 2019

Thanks for the input! I'll definitely take a look at your acceptance tests and make sure I cover all of them. Any future input or advice as I expand upon the library would be greatly appreciated.

@igorbernstein2
Copy link
Author

If you plan on implementing retries, here are a few things to watch out for:

  • For ReadRows make sure to track both the last merged row key and the last_scanned_row_key. So that you can skip over any filtered out rows on stream resumption
  • There is a gotcha with ServerStreaming RPCs, where they can send all of their data but return an error status code. This is particularly dangerous with ReadRows, where if you aren't careful the resumption request can end up being empty triggering a full table scan.
  • Try to avoid using serverside timestamps. They prevent mutations from being idempotent

In terms of default timeouts and retry counts, I would recommend to track gapic configs.

Unfortunately, since I'm not familiar with elixir, I don't have anything more specific. If you have any further questions, feel free to ping me.

@jscott22 jscott22 self-assigned this Feb 6, 2019
@jscott22 jscott22 added bug Something isn't working enhancement New feature or request labels Feb 6, 2019
@jscott22 jscott22 pinned this issue Feb 6, 2019
@jscott22 jscott22 added this to the 1.0.0 milestone Feb 11, 2019
@jscott22 jscott22 unpinned this issue Feb 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants