Skip to content

Conversation

@naung9
Copy link
Contributor

@naung9 naung9 commented Nov 15, 2024

Shutdown bqReadClient after arrow stream is processed in highThroughputRead to free up resources

Fixes #3571

@naung9 naung9 requested a review from a team as a code owner November 15, 2024 01:14
@naung9 naung9 requested a review from hongalex November 15, 2024 01:14
@product-auto-label product-auto-label bot added size: s Pull request size is small. api: bigquery Issues related to the googleapis/java-bigquery API. labels Nov 15, 2024
@naung9 naung9 changed the title Close bq read client fix: Close bq read client Nov 15, 2024
@PhongChuong PhongChuong added the kokoro:run Add this label to force Kokoro to re-run the tests. label Jan 7, 2025
@PhongChuong
Copy link
Contributor

/gcbrun

@yoshi-kokoro yoshi-kokoro removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Jan 7, 2025
@PhongChuong PhongChuong added owlbot:run Add this label to trigger the Owlbot post processor. and removed owlbot:run Add this label to trigger the Owlbot post processor. labels Jan 8, 2025
@PhongChuong
Copy link
Contributor

Hi @naung9 ,
Sorry for the delay in reviewing this PR. The PR looks good. However, due to policy, forked repository contributions are considered untrusted.

Workaround: Have the PR target a non-main branch in the googleapis/java-bigquery repo. Merge into that branch. Open a PR from that "trusted" branch into main.

If this is too much trouble, let me know if you want me to create the branch/PR for you with your changes and attribute the code to you in the PR description.

@naung9
Copy link
Contributor Author

naung9 commented Jan 9, 2025

Hi @PhongChuong , sorry to bother you. But I don't have permission to create a new branch in this Repo so please create a new branch for me to target my PR into. Or if you want me to target a specific existing branch, please tell me the branch name. I see 3 active branches in this repo execute-select, i3626, release-please--branches--main.

@PhongChuong
Copy link
Contributor

@naung9 , let me update our instruction on how to contribute/push to main and get back to you soon (a day or 2). Our current instruction for contributors is lacking in this regard. Sorry for all the delays.

@naung9
Copy link
Contributor Author

naung9 commented Jan 9, 2025

@PhongChuong Sure. Take your time. I'll wait.

@PhongChuong
Copy link
Contributor

@naung9 , I'm trying working on getting the process hammered out. Mean while, can you please check to see if you have permissions to push to this newly created branch: closeReadClient?
Testing with my non-Google account seems to indicate no but I would like to confirm.

@PhongChuong
Copy link
Contributor

@naung9 , this process seems to work:

  1. Create a fork of java-bigquery
  2. Commit your changes to the fork
  3. From your branch, "contribute" to java-bigquery which creates a PR that will go through the normal review process.

@naung9
Copy link
Contributor Author

naung9 commented Jan 15, 2025

@PhongChuong I tried pushing directly to the closeReadClient branch and got denied so your protection rule is working as intended.

From your branch, "contribute" to java-bigquery which creates a PR that will go through the normal review process.

Does it mean I have to create a new PR from my fork that points to closeReadClient or to main branch directly?

@PhongChuong
Copy link
Contributor

@PhongChuong I tried pushing directly to the closeReadClient branch and got denied so your protection rule is working as intended.

From your branch, "contribute" to java-bigquery which creates a PR that will go through the normal review process.

Does it mean I have to create a new PR from my fork that points to closeReadClient or to main branch directly?

The fork should point to 'main' directly.

@naung9
Copy link
Contributor Author

naung9 commented Jan 15, 2025

@PhongChuong I created a new PR here. So you can close this PR and continue on the new one.

@PhongChuong
Copy link
Contributor

@naung9 , running tests on the new PR now. Thanks for all your patience with the process. Closing this PR.

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 googleapis/java-bigquery API. size: s Pull request size is small.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

BigQuery: BigQueryReadClient is not shutdown properly in Connection API

3 participants