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

Retry query jobs and use info-level logging for job link, etc #768

Closed
wants to merge 5 commits into from
Closed

Retry query jobs and use info-level logging for job link, etc #768

wants to merge 5 commits into from

Conversation

tbog357
Copy link

@tbog357 tbog357 commented Jun 11, 2023

resolves #696

Description

Improve productivity, provide the Bigquery job's link just after it's submitted. Make it easier to find the executed queries

Example of logs proposal
image

Checklist

@tbog357 tbog357 requested a review from a team as a code owner June 11, 2023 10:22
@tbog357 tbog357 requested a review from colin-rogers-dbt June 11, 2023 10:22
@cla-bot
Copy link

cla-bot bot commented Jun 11, 2023

Thanks for your pull request, and welcome to our community! We require contributors to sign our Contributor License Agreement and we don't seem to have your signature on file. Check out this article for more information on why we have a CLA.

In order for us to review and merge your code, please submit the Individual Contributor License Agreement form attached above above. If you have questions about the CLA, or if you believe you've received this message in error, please reach out through a comment on this PR.

CLA has not been signed by users: @tbog357

@tbog357 tbog357 marked this pull request as draft June 11, 2023 10:24
@tbog357 tbog357 marked this pull request as ready for review June 11, 2023 10:25
@tbog357 tbog357 marked this pull request as draft June 11, 2023 10:45
@tbog357 tbog357 marked this pull request as ready for review June 11, 2023 10:45
@cla-bot cla-bot bot added the cla:yes label Jun 11, 2023
Copy link
Contributor

@github-christophe-oudar github-christophe-oudar left a comment

Choose a reason for hiding this comment

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

Hello,
Thanks for raising the PR!
I've implemented most of it in #630.
Yet you're adding few things in this one that could be making it more worthwhile:

  • retries on the query job call with an error counter
  • a link that includes the attempts, materialization, incremental resource type & node name
  • add it as an info log

I think your approach is great but it might require some changes:

  • Do you think we could align the info in the error log too? I think it's fine to log it twice to easily find the job link in case of error.
  • the error counter starts at 0 which is kind of weird to me, what about making it start at 1 and stop after 2 retries (so 3 total)? Maybe even make it a configurable thing but I guess it's more work than needed so far.
  • Is it catching all errors? For instance, if I have a SQL or runtime BQ error and it fails, will it start and retry it on every dbt run? I think most people would rather avoid that behavior. So I'm bit worried about introducing that retry especially since 1.6 introduces already dbt retry.
  • If you're adding this log, you should remove https://github.com/dbt-labs/dbt-bigquery/blob/main/dbt/adapters/bigquery/connections.py#LL537C4-L537C4 as it would be obsolete then

@dbeatty10 dbeatty10 changed the title Log submitted Bigquery job Retry query jobs and use info-level logging for job link, etc Jul 12, 2023
@@ -0,0 +1,6 @@
kind: Features
body: Provide bigquery job link after it's submitted
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm going to proactively update the changelog entry for this PR to be more precise. I will commit (rather than just suggest) with the intent that we can improve it further as-needed.

Suggested change
body: Provide bigquery job link after it's submitted
body: Retry query jobs and use info-level logging for job link, etc

Copy link
Contributor

Choose a reason for hiding this comment

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

Update: I can't push commits to this branch, so I'll just make this a blocking review instead.

Copy link
Contributor

@dbeatty10 dbeatty10 left a comment

Choose a reason for hiding this comment

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

@tbog357 A handful of things for you to do:

  1. Open a feature request for converting from debug-level to info-level (c.f. here)
  2. Open a feature request for retrying query jobs
  3. Update the changelog entry to reflect those two feature request issue numbers (and removing 696 which was already resolved)
  4. Update the changelog entry description here

Both of those feature requests will need to be approved by our product team (i.e., @Fleid or @dataders)

@tbog357
Copy link
Author

tbog357 commented Jul 15, 2023

Hi @dbeatty10 , thanks for reminding me

  1. I think logging the job-link at the debug-level is reasonable.

  2. This PR doesn't try to implement a retry feature, it just shows the count of retries if profiles.yml have job_retries configuration https://docs.getdbt.com/docs/core/connect-data-platform/bigquery-setup#job_retries

But the PR of @github-christophe-oudar (#697) suited my need. So I will close this PR

Thanks all

@tbog357 tbog357 closed this Jul 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ADAP-513] [Feature] Move the BQ Job link after the job submission instead of job done
3 participants