Skip to content

Keep polling in the BigQuery check triggers while a job is running - #74305

Merged
shahar1 merged 1 commit into
apache:mainfrom
bingqin2:bigquery-check-triggers-running
Oct 6, 2026
Merged

shahar1 merged 1 commit into
apache:mainfrom
bingqin2:bigquery-check-triggers-running

Conversation

@bingqin2

@bingqin2 bingqin2 commented Oct 5, 2026

Copy link
Copy Markdown
Contributor

BigQueryValueCheckTrigger and BigQueryIntervalCheckTrigger treated only "pending" as in progress. BigQueryAsyncHook.get_job_status() returns the job state as it is until the job is DONE, so a query that is executing reports "running", and both triggers sent that to the error branch. In deferrable mode, BigQueryValueCheckOperator and BigQueryIntervalCheckOperator therefore failed with Job running whenever the query outlived one poll interval (#73981).

Both triggers now follow the order the other triggers in the module already use: success, then error, then keep polling for any other status. The hook only returns success, error, or the lowercased BigQuery job state (pending / running), so the polling branch cannot spin on an unexpected terminal status.

For the interval check this also fixes which job's message is reported. The error branch always used the second job's message, so a failed first job surfaced as Job completed; and while one job had failed and the other was still pending, the trigger kept polling until the other finished. It now fails as soon as either job has failed, with that job's message.

Changes

  • providers/google/src/airflow/providers/google/cloud/triggers/bigquery.py: BigQueryValueCheckTrigger.run and BigQueryIntervalCheckTrigger.run keep polling unless the job succeeded or failed; the interval check reports the failed job
  • providers/google/tests/unit/google/cloud/triggers/test_bigquery.py: test_interval_check_trigger_keeps_polling_while_a_job_is_running (first, second or both jobs running), test_interval_check_trigger_reports_the_failed_job (first failed, second failed, first failed while the second is still running) and test_value_check_op_trigger_keeps_polling_while_job_is_running. Six of the seven cases fail on main; the "second failed" case guards that the second job's message is still used when it is the one that failed. The polling tests patch asyncio.sleep and feed a status sequence, so they do not depend on timing.

Testing

  • providers/google: tests/unit/google/cloud/triggers/test_bigquery.py (57 passed)
  • mypy on the changed module, prek hooks on the changed files
  • The deferrable value and interval checks run in providers/google/tests/system/google/cloud/bigquery/example_bigquery_queries_async.py; I don't have a GCP project to run it myself.

closes: #73981


Was generative AI tooling used to co-author this PR?
  • Yes (please specify the tool below)

Generated-by: Claude Code (Claude Opus 5.5) following the guidelines. I reviewed and understand all changes; the tests were run locally as listed above.


🤖 Generated with Claude Code

@bingqin2
bingqin2 requested a review from shahar1 as a code owner October 5, 2026 22:59
@boring-cyborg boring-cyborg Bot added area:providers provider:google Google (including GCP) related issues labels Oct 5, 2026
@shahar1
shahar1 merged commit eda8563 into apache:main Oct 6, 2026
87 checks passed
@molcay

molcay commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

Hi @shahar1,

Could we please revert this change?

This PR was merged without any system tests running and did not receive an approval from the our team. Given the nature of the code, the changes require closer scrutiny and shouldn't be merged as-is without a thorough review.

As we recently noted on the mailing list (https://lists.apache.org/thread/7z1q39d8fq6ny9jj6omxkrk9o1mogmk7), we require changes to the GCP provider to be tested against real GCP resources. Catching issues during the RC phase doubles or triples our workload, so we need to ensure this goes through the proper process first.

@shahar1

shahar1 commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

Hi @shahar1,

Could we please revert this change?

This PR was merged without any system tests running and did not receive an approval from the our team. Given the nature of the code, the changes require closer scrutiny and shouldn't be merged as-is without a thorough review.

As we recently noted on the mailing list (https://lists.apache.org/thread/7z1q39d8fq6ny9jj6omxkrk9o1mogmk7), we require changes to the GCP provider to be tested against real GCP resources. Catching issues during the RC phase doubles or triples our workload, so we need to ensure this goes through the proper process first.

Hey Olcay, I reviewed this PR this morning with magpie to try and include this fix in today's providers release.
Before merging, I haden't believed that this specific case required to run the system tests - which might have been a wrong decision, for which I apologize.
However, as long as we don't have an automated process of system tests that is integrated as part of the CI/CD,
requiring both system tests to run and your team's approval altogether slows down the reviewing process significantly, and eventually puts the burden on maintainers / release managers.
As I was the one who merged it, I'll take responsibility for either fixing or reverting it if required, as follows:

  1. Before trying to revert it, I'll work on the system tests on my own end. If they pass successfully - I'll just create the PR to insert them.
  2. If there are any issues related to this commit, I'll vote -1 in the upcoming release and take care of an ad-hoc release if necessary which either reverts the PR or fixes it.

I'll be more cautious next time, but I'll appreciate your understanding as for my end.
I'll respond to the dev list thread later on (for some reason didn't get it on my email).
I'll be happy to cooperate in anything related to the integration of the system tests as part of the CI/CD.

@molcay

molcay commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

Hi @shahar1,

First off, I want to sincerely apologize if my previous comment came across as harsh—that wasn't my intention at all. My main goal was to highlight a broader, ongoing challenge we are facing on our end, rather than making a strict point about this specific PR.

I completely understand your perspective. You are absolutely right that requiring manual system tests and waiting for our team's explicit approval can create bottlenecks, slow down the release cycle, and put an unfair burden on maintainers and release managers. At the same time, when untested changes make it to the RC phase, it creates significant downstream problems and triples the workload to catch and fix issues late in the game. Currently, we are trying to make internal arrangements to review PRs more quickly.

I think this situation is a great signal that we need to find a better middle ground. We should work together to establish a sustainable process that works for everyone involved—our team, the Airflow maintainers, and the contributors.

Thank you for taking ownership of running the tests for this PR in the meantime; I really appreciate it. Let's definitely keep the conversation going on the dev list about how we can better automate and integrate system tests into the CI/CD. Until automation is complete, maybe we can find another way to share this load somehow.

Ultimately, we all share the exact same goal: making Airflow and all providers (not only the google provider) as stable and reliable as possible.

Thanks again for your understanding and collaboration!

@bingqin2

bingqin2 commented Oct 6, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @shahar1 for taking this on, and @molcay for raising it; you're right that this should have been run against real BigQuery before review. I'm setting up a GCP project and will post a run of example_bigquery_queries_async with this change here (it covers both deferrable checks), so you don't have to pull the branch. My Google provider PRs will include real-GCP evidence from now on, as proposed on the dev list.

@bingqin2
bingqin2 deleted the bigquery-check-triggers-running branch October 6, 2026 19:12
@potiuk

potiuk commented Oct 7, 2026

Copy link
Copy Markdown
Member

Thanks @shahar1 for taking this on, and @molcay for raising it; you're right that this should have been run against real BigQuery before review. I'm setting up a GCP project and will post a run of example_bigquery_queries_async with this change here (it covers both deferrable checks), so you don't have to pull the branch. My Google provider PRs will include real-GCP evidence from now on, as proposed on the dev list.

Yeah. I also let some of those slip. And we have to figure out better way - soon I hope.

Note @molcay @VladaZakharova -> for whatever reason your message is in devlist archive but at least few people did not receive it (not even in SPAM). This is a second time I saw it happen, so I am going to raise to ASF Infra as a ticket - because there is something fishy about the delivery of those messages.

@VladaZakharova

Copy link
Copy Markdown
Contributor

Thanks @shahar1 for taking this on, and @molcay for raising it; you're right that this should have been run against real BigQuery before review. I'm setting up a GCP project and will post a run of example_bigquery_queries_async with this change here (it covers both deferrable checks), so you don't have to pull the branch. My Google provider PRs will include real-GCP evidence from now on, as proposed on the dev list.

Yeah. I also let some of those slip. And we have to figure out better way - soon I hope.

Note @molcay @VladaZakharova -> for whatever reason your message is in devlist archive but at least few people did not receive it (not even in SPAM). This is a second time I saw it happen, so I am going to raise to ASF Infra as a ticket - because there is something fishy about the delivery of those messages.

Omg, i didn't even notice that the message got archived. I was wondering why noone was replying :D
i will send it also in dev channel in Slack for better visibility

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providers provider:google Google (including GCP) related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

BigQueryValueCheckTrigger and BigQueryIntervalCheckTrigger fail while the BigQuery job is still running

5 participants