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

Remove destination table from BigQuery config #171

Merged

Conversation

mayaCostantini
Copy link
Contributor

@mayaCostantini mayaCostantini commented Jun 15, 2022

This introduces a breaking change

  • No

This should yield a new module release

  • Yes

This Pull Request implements

Remove BigQuery destination table in pypi-downloads handler. Writing to a destination table does not seem to have any utility here and might be the source of a lot of traffic generated on the BigQuery API (~6000 calls to the google.cloud.bigquery.v2.JobService.InsertJob API method).

@sesheta sesheta requested review from codificat and KPostOffice June 15, 2022 12:17
@sesheta sesheta added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jun 15, 2022
@mayaCostantini mayaCostantini force-pushed the remove-destination-table branch from f2f863c to fa7d824 Compare June 15, 2022 12:31
Copy link
Member

@codificat codificat left a comment

Choose a reason for hiding this comment

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

Nit: currently the PR is a one line change but there are 2 commits; better squash them into 1.

Besides that, this LGTM, but I must admit I'm not sure I get its purpose 😇 could you elaborate a bit on the context/why?
Thanks!

@mayaCostantini mayaCostantini force-pushed the remove-destination-table branch from fa7d824 to 156b513 Compare June 15, 2022 14:42
@mayaCostantini
Copy link
Contributor Author

@codificat thanks for the review, I updated the PR description and squashed the commits

Copy link
Member

@harshad16 harshad16 left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@sesheta sesheta added the lgtm Indicates that a PR is ready to be merged. label Jun 17, 2022
@sesheta
Copy link
Member

sesheta commented Jun 17, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: harshad16

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@sesheta sesheta added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 17, 2022
@sesheta sesheta merged commit e188712 into thoth-station:master Jun 17, 2022
@mayaCostantini mayaCostantini deleted the remove-destination-table branch June 17, 2022 05:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants