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

add support for bigquery AppendRows storage write API #95

Merged
merged 6 commits into from
Jul 7, 2024

Conversation

imor
Copy link
Contributor

@imor imor commented Jun 30, 2024

NOTE: review and merge the following PR first: #94. This is because this PR's branch is cut from the previous PR's branch.

This PR adds support for AppendRows API. It adds googleapis repo as a submodule to use .proto files from it. prost-build (via the tonic-build dependency) is used to compile the .proto files into Rust code. Since the storage write API is a gRPC API, tonic is added as a dependency to use gRPC in Rust.

Partially fixes #53.

@lquerel
Copy link
Owner

lquerel commented Jul 2, 2024

Thank you for this contribution. I am on vacation until the beginning of next week. I will review this PR when I return.

@imor imor marked this pull request as ready for review July 4, 2024 05:49
Copy link
Owner

@lquerel lquerel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for investing the necessary time to add support for the Storage Write API, which was highly requested by the users of this crate and something I never had the time to integrate. I will just add a unit test in addition to the example you provided.

@lquerel lquerel merged commit d79313d into lquerel:main Jul 7, 2024
0 of 3 checks passed
@lquerel
Copy link
Owner

lquerel commented Jul 8, 2024

I will change the way the proto files are integrated in this PR. I am not a fan of integration via git submodule as it tends to import too many things and makes contributions more complex. However, thank you again for this contribution.

@imor imor deleted the append_rows branch July 8, 2024 11:04
@lquerel
Copy link
Owner

lquerel commented Jul 28, 2024

@imor Hi. Could you please verify on your end that the main branch is working correctly with your service account? On my side, although I think I have set the necessary permissions, I am now getting the following error when connecting to the service for the storage API (the grpc part) in the unit test present in table.rs (also true for other tests). I think it worked before, but I don’t understand what has happened in the meantime. As I don’t have much time to devote to this project at the moment, I would appreciate your help with this contribution. Thank you.

Error: TonicTransportError(tonic::transport::Error(Transport, ConnectError(Custom { kind: InvalidData, error: InvalidCertificate(UnknownIssuer) })))

If it works on your side, could you list the permissions you used so I can compare them with what I have?

@imor
Copy link
Contributor Author

imor commented Jul 29, 2024

Thanks for spending time on this issue @lquerel, I greatly appreciate it. I have been testing this branch's code since the past few weeks and it works for me flawlessly. The error looks like a low level transport level error, so I don't think it's a service account permissions issue. The InvalidCertificate(UnknownIssuer) part of the error indicates the client can't verify the certificate from the server. Does the environment where you see this failure have up to date root certificates? If it's a debian based env you might need something like apt install ca-certificates or similar for other environments.

@fungiboletus
Copy link

I pulled the main branch and I get the same error on an up to date MacOS 14.5. However it does fail before using the storage API, so it may be unrelated to this pull request.

@fungiboletus
Copy link

Looks like the issue appears in b0b84ec (tonic update from 0.11 to 0.12 is my guess).

@imor
Copy link
Contributor Author

imor commented Aug 7, 2024

Thanks @fungiboletus for testing with an older tonic version. It turned out there was a breaking change in version 0.12. I've raised a PR to fix this: #107.

@imor
Copy link
Contributor Author

imor commented Aug 7, 2024

@lquerel once that new PR is merged, it should unblock your testing and help you review #97 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support BigQuery Storage API
3 participants