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

Refactor dataproc system tests #40720

Merged
merged 1 commit into from
Jul 24, 2024
Merged

Conversation

VladaZakharova
Copy link
Contributor


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@boring-cyborg boring-cyborg bot added area:providers area:system-tests provider:google Google (including GCP) related issues labels Jul 11, 2024
@potiuk
Copy link
Member

potiuk commented Jul 11, 2024

Some static checks. As usuall - I recommend to have pre-commit installed, it fixes all those problems automatically :)

Copy link
Contributor

@shahar1 shahar1 left a comment

Choose a reason for hiding this comment

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

LGTM overall - static checks should be fixed as Jarek mentioned.
Also, I assume that there was a reason for changing the regions in the system tests - is there any policy for that? (I think that it should be mentioned somwhere)

@VladaZakharova VladaZakharova force-pushed the dataproc-sys-tests branch 2 times, most recently from 6ad0951 to bf35f93 Compare July 23, 2024 11:56
@VladaZakharova
Copy link
Contributor Author

@potiuk @shahar1 hi! thank you for checking this one. Changes in regions and locations were made only to fix the problem in our Composer CI: with previous setup there was a big load on one specific region that was used in all of them. This PR fixes this, but the rest in the test remains the same.
I will try to fix static checks, but there is one that I don't know how to :)
In dataproc_batch we have task that runs cancel_operation() method to cancel any long-running operation. In this case it should be run in parallel with the task that performs that long-running operation (create_batch_4) and since the operation was cancelled, the task itself is failed. Watcher sees the failed task and marks the whole DAG as failed. I was trying to exclude this create_task from the list for watcher, but static check is failing because of that. Do you have some ideas maybe how I can fix it?

@potiuk
Copy link
Member

potiuk commented Jul 23, 2024

I will try to fix static checks, but there is one that I don't know how to :)

In this pre-commit:

index = content.find(WATCHER_APPEND_INSTRUCTION)

Just have a `>> watcher()`` check instead of the full append instructions

@potiuk potiuk merged commit 95b5a0a into apache:main Jul 24, 2024
53 checks passed
molcay pushed a commit to VladaZakharova/airflow that referenced this pull request Aug 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers area:system-tests provider:google Google (including GCP) related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants