-
Notifications
You must be signed in to change notification settings - Fork 17.4k
Resolve Beam Dataflow job id by name after launcher returns #67711
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
Closed
evgeniy-b
wants to merge
1
commit into
apache:main
from
evgeniy-b:fix-beam-dataflow-job-id-resolve-by-name
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@evgeniy-b as I understand in case when users run in parallel 2 or more Jobs with the same name or on Dataflow the Job with this name already present than this code returns
Noneas JobID value, please correct me if I am wrong?In the current logic with callbacks the code parse Apache Beam logs for availability of JobID and when getting it then starts the waiting process in deferrable or non-deferable mode. It means that we always have unique Job ID.
This new logic looks for me as a breaking change because returns
Noneas JobID in case when in Dataflow the users have 2 or more Jobs with the same name. It is possible scenario for the most of our users because in Dataflow is impossible to remove finished Jobs the user can only archived it. And our_fetch_all_jobsmethod does not sort Jobs by finished or running and returns all Jobs with the same name.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me explain a bit how I arrived here. On an airflow cluster I maintain I noticed python beam jobs running with
deferrable=False, so I switched that flag to true to not waste worker resources. On the next day the jobs failed while transitioning to async triggers because their STDOUT didn't contain the job ID. In the sync mode a missing job ID doesn't prevent the task from succeeding:_DataflowJobsController.wait_for_donepollsself._refresh_jobs():airflow/providers/google/src/airflow/providers/google/cloud/hooks/dataflow.py
Lines 532 to 542 in a7174b5
_refresh_jobscallsself._get_current_jobs():airflow/providers/google/src/airflow/providers/google/cloud/hooks/dataflow.py
Lines 465 to 471 in a7174b5
_get_current_jobs— with no_job_id— callsself._fetch_jobs_by_prefix_name(self._job_name.lower()):airflow/providers/google/src/airflow/providers/google/cloud/hooks/dataflow.py
Lines 328 to 339 in a7174b5
_fetch_jobs_by_prefix_namecallsself._fetch_all_jobs()and returns every prefix-matched job (archived + running, no terminal-state filter):airflow/providers/google/src/airflow/providers/google/cloud/hooks/dataflow.py
Lines 460 to 463 in a7174b5
So today's sync path already silently picks up every prefix-matched job whenever the regex misses.
With default
append_job_name=Truethe job name will be unique and job ID will be retrieved.But you are right, it is a degradation: for jobs without unique names but printing out their IDs to console, the job ID will become missing.
I guess an alternative could be to replicate the sync mode's behavior in the async path which currently fails without
job_id. However it means that xcom and a link to the job will stay broken.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think job ID in output detection should be reverted. While it is awkward in principle, it is the only way (?) to reliably get ID when job names are not unique. Then name-based ID detection can be used as a fallback but only when
append_job_name=True. And if the trigger receives empty job ID it should fallback to polling status of all jobs matching the name (and not in terminal status).@MaksYermak what's your take on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@evgeniy-b I do not like idea using job name for checking job status, because, as I already mentioned, is not unique and all manipulation with a code looks like workarounds when we try to introduce additional parameters for making job name kind unique, but it still not.
For example when user start two parallel tasks with Jobs which will have the same Job name and unique JobIDs for this case what Job this code grab for checking the status? As I understand not a single one or, maybe, the first JobID from the job list then both task will monitor the same job which is wrong. I do not see any solution how we can distinguish two Job with the same name between the tasks in parallel run and how task should understand what Job to pick. This solution with callbacks was introduced in the beginner of life for Apache Beam operators and removing it completely is breaking change for users.
About problem which you mentioned.
What version of Apache Beam provider do you use on your Airflow cluster? Because problem which you described should not happened because of this code. This code use callback for getting Job ID from STDOUT for Dataflow runner before stating to wait in non-deferrable or deferrable modes. It means that changing value for
deferrableflag fromFalsetoTruedoes not apply to callback logic at all, because the code always use callbacks for Dataflow runner. And only after getting Job ID decides in what mode wait for result in deferrable or non-deferrable.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that job names are not unique and totally agree that using names for status checks is awkward.
Fair. I'm not proposing to remove it anymore because it would be a regression.
It works only when STDOUT contains the job ID. In my case the job's output didn't include it. So
jobId=None. The divergence is in how deferrable/worker mode treat missing job IDs. Trigger fails immediately while the sync worker path lists all jobs by the name and monitors statuses of all matching jobs (I linked exact code lines in the previous comment).It's a real bug: all tasks in deferrable mode whose job outputs didn't match Job ID regex will fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I am still do not understand how it can be possible, in your case, to start deferrable mode without
JobID. Because in the current code we have this logic forprocess_fdand this logic forrun_beam_command. As you can see, in the code we havewhile Trueloop which reads logs from Beam run process till the Job finished. And only in case, whenJobIDpresents the code leaves this loop and starts waiting process using Dataflow API viadeferrableornon-deferrablemodes. Otherwise, if you do not haveJobIDthen the code runs your Job innon-deferrablemode till the end and never use Dataflow API for checkin status.I see only one scenario when the fail in
deferrablemode can be possible when withoutJobIDthis infinite loop goes to the end and successfully finished the Job. And after that Operator tries to startdeferrablemode and failed because theJobIDis empty. And innon-deferrablemode everything is fine because for wait_for_done, theJobIDcan beNone. I think this can be your's scenario, but I need equivalent of your's Pipeline script for reproduction.Could you please share Apache Beam provider version which you use and the code for reproduction this issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. I think this is exactly what did happen. It matches the logs: job starts at 03:19, completes at 04:49 and defers. The error is raised only at 05:30 because the task runs in a pool with limited concurrency.
I can prepare an example to reproduce. Should I open it as a new bug ticket?
I can also create a PR right away if we agree on the fix approach. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@evgeniy-b in my opinion it makes sense to create an airflow issue with all reproduction steps and then continue discussion about fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense! Here is the ticket #68279