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

Fixes an issue with splitReadStream #6112

Closed
wants to merge 2 commits into from
Closed

Conversation

aryann
Copy link

@aryann aryann commented Aug 20, 2019

The method requires a fraction, which is not provided in this client.

Additionally, I'm removing some comments that don't make sense.

aryann added 2 commits August 20, 2019 11:36
All of these methods were handwritten.
The server denies requests without a fractional value set. This change sets a
semi-sensible default. In the future we should remove this method and force
users to use the one with a request message since users should know what
fraction they want to split at (i.e., 0.5 is not always a good choice).
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Aug 20, 2019
@chingor13
Copy link
Contributor

Thanks for submitting this. The comments mean that this entire file is managed by a code generator, so any hand-written updates will be overwritten. We will need to address this upstream in our generator and/or service definitions.

Can you open an issue here to reference and we will look into how to best make the changes upstream?

@aryann
Copy link
Author

aryann commented Aug 21, 2019

I was told my @mmladenovski that this file was written by hand, which is why I removed the comments.

How is this file generated?

@mmladenovski
Copy link

mmladenovski commented Aug 21, 2019

I was told my @mmladenovski that this file was written by hand, which is why I removed the comments.

How is this file generated?

@aryann , the file was added with #4022 . @kmjung , might provide more details.

I think it was copied from the auto-generated BaseBigQueryStorageClient and then modified to use the Enhanced stub along with the ResumptionStrategy. Currently the file is manually maintained.

The comments related to auto-generation should be removed.

@chingor13 chingor13 added the api: bigquery Issues related to the BigQuery API. label Aug 26, 2019
SplitReadStreamRequest.newBuilder().setOriginalStream(originalStream).build();
SplitReadStreamRequest.newBuilder()
.setOriginalStream(originalStream)
.setFraction(0.5f)

Choose a reason for hiding this comment

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

I would suggest to move the 0.5f value to a constant STREAM_SPLIT_FRACTION and use it in all places!

@meredithslota
Copy link

The BigQuery Storage client for Java now lives here: https://github.com/googleapis/java-bigquerystorage and has recently been re-generated. The comments re: auto-generation are still present because this is an autogenerated library, but if you see other issues (the file referenced from v1beta1 is here: https://github.com/googleapis/java-bigquerystorage/blob/master/google-cloud-bigquerystorage/src/main/java/com/google/cloud/bigquery/storage/v1beta1/BigQueryStorageClient.java) please file an issue in that repo. I'm going to go ahead and close this.

cc: @stephaniewang526 as an FYI and in case you want to do something different with this issue. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigquery Issues related to the BigQuery API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants