AWSGlueJobHook updates job configuration if it exists #27893

romibuzi · 2022-11-24T15:01:27Z

Rename GlueJobHook.get_or_create_glue_job() into create_or_update_glue_job() and split code into different methods: create_glue_job_config(), has_job(), create_job() and update_job().

It is now similar to the behavior in GlueCrawlerOperator.

boring-cyborg · 2022-11-24T15:01:31Z

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:

Pay attention to the quality of your code (flake8, mypy and type annotations). Our pre-commits will help you with that.
In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
Consider using Breeze environment for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
Be sure to read the Airflow Coding style.
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: [email protected]
Slack: https://s.apache.org/airflow-slack

Taragolis · 2022-11-24T17:53:31Z

airflow/providers/amazon/aws/hooks/glue.py

+    @cached_property
+    def glue_client(self):
+        """:return: AWS Glue client"""
+        return self.get_conn()


small nitpick AwsBaseHook already have two ways how to get boto3.client:

AwsBaseHook.get_conn()

AwsBaseHook.conn

And both of them cached so... might be better not to create third way?

Oh good catch! I have removed it and made use of AwsBaseHook.get_conn() instead

syedahsn · 2022-11-24T18:25:13Z

airflow/providers/amazon/aws/hooks/glue.py

+        execution_role = self.get_iam_execution_role()
+
+        if "WorkerType" in self.create_job_kwargs and "NumberOfWorkers" in self.create_job_kwargs:
+            return dict(


This part here can be refactored to be a bit more concise. Rather than have two return statements returning very similar dictionaries, something like this would be cleaner:

ret_config = { "Name": self.job_name, "Description": self.desc, "LogUri": s3_log_path, "Role": execution_role["Role"]["Arn"], "ExecutionProperty": {"MaxConcurrentRuns": self.concurrent_run_limit}, "Command": command, "MaxRetries": self.retry_limit, **self.create_job_kwargs, } if "WorkerType" in self.create_job_kwargs and "NumberOfWorkers" in self.create_job_kwargs: ret_config["MaxCapacity"] = self.num_of_dpus return ret_config

Also, it's generally preferable to use {} rather than the dict() function

Done! indeed this way it is nicer

romibuzi · 2022-12-01T14:52:58Z

@Taragolis and @syedahsn Thanks for your reviews! I have done modifications. Feel free to take a look at them :)

Taragolis

looks good to me

vincbeck · 2022-12-02T20:45:16Z

airflow/providers/amazon/aws/hooks/glue.py

@@ -92,10 +92,38 @@ def __init__(
        kwargs["client_type"] = "glue"
        super().__init__(*args, **kwargs)

+    def create_glue_job_config(self) -> dict:
+        if self.s3_bucket is None:
+            raise AirflowException("Could not initialize glue job, error: Specify Parameter `s3_bucket`")


TypeError would make more sense?

done, i have checked in other AWS hooks and it is usually ValueError raised in case a parameter is missing so i put this one

vincbeck · 2022-12-02T20:47:14Z

airflow/providers/amazon/aws/hooks/glue.py

+            raise AirflowException("Could not initialize glue job, error: Specify Parameter `s3_bucket`")
+
+        default_command = {
+            "Name": "glueetl",


Should this name provided by the user and not hardcoded?

here it is the default command used but the user can specify another one with the "Command" kwarg: https://github.com/apache/airflow/blob/23d33fe4f424b0302a1637a6005185ea4139c04e/airflow/providers/amazon/aws/hooks/glue.py#L103

I see, you can ignore this comment then

vincbeck · 2022-12-02T20:49:55Z

airflow/providers/amazon/aws/hooks/glue.py

+            "Name": self.job_name,
+            "Description": self.desc,
+            "LogUri": s3_log_path,
+            "Role": execution_role["Role"]["Arn"],


It is a pretty strong assumption to assume that the user wants the execution role as role no? Should not it be specified by the user?

execution_role here is the one associated with the role_name argument of the hook: https://github.com/apache/airflow/blob/23d33fe4f424b0302a1637a6005185ea4139c04e/airflow/providers/amazon/aws/hooks/glue.py#L134-L136

vincbeck · 2022-12-02T20:53:08Z

airflow/providers/amazon/aws/hooks/glue.py

@@ -231,54 +256,72 @@ def job_completion(self, job_name: str, run_id: str, verbose: bool = False) -> d
                        next_token=next_log_token,
                    )

-    def get_or_create_glue_job(self) -> str:
+    def get_job(self, job_name) -> dict:


As a convention we usually try to avoid having methods in hooks which just are just wrapper around boto3 APIs. To me, there is no much value having get_job function in the hook since you can use directly hook.conn.get_job

Good remark indeed it doesnt make much sense if there is no additionnal logic in these wrappers methods, I have removed them

vincbeck · 2022-12-02T20:56:13Z

airflow/providers/amazon/aws/hooks/glue.py

+        else:
+            return False
+
+    def create_job(self, **job_kwargs) -> str:


Same comment here, this function is just a wrapper around boto3 create_job API

romibuzi · 2022-12-06T09:00:04Z

Thank you @vincbeck!!

boring-cyborg · 2022-12-06T14:29:25Z

Awesome work, congrats on your first merged pull request!

romibuzi requested a review from eladkal as a code owner November 24, 2022 15:01

boring-cyborg bot added area:providers provider:amazon-aws AWS/Amazon - related issues labels Nov 24, 2022

Taragolis reviewed Nov 24, 2022

View reviewed changes

syedahsn reviewed Nov 24, 2022

View reviewed changes

Taragolis self-requested a review December 1, 2022 16:53

Taragolis approved these changes Dec 1, 2022

View reviewed changes

syedahsn approved these changes Dec 1, 2022

View reviewed changes

vincbeck requested changes Dec 2, 2022

View reviewed changes

vincbeck approved these changes Dec 5, 2022

View reviewed changes

Romain Ardiet added 3 commits December 6, 2022 08:50

AWSGlueJobOperator updates job configuration if it exists

399fa3a

Review

e74807b

Review - remove wrappers methods create_job and get_job in AWS Glue hook

2cb3d3e

potiuk approved these changes Dec 6, 2022

View reviewed changes

eladkal approved these changes Dec 6, 2022

View reviewed changes

eladkal merged commit b609ab9 into apache:main Dec 6, 2022

romibuzi deleted the feature/27592-aws-glue-job-update branch December 6, 2022 14:48

eladkal mentioned this pull request Jan 2, 2023

Status of testing Providers that were prepared on January 02, 2023 #28679

Closed

84 tasks

vgutkovsk mentioned this pull request Feb 8, 2023

GlueJobOperator throws error after migration to newest version of Airflow #29423

Closed

2 tasks

o-nikolas mentioned this pull request Mar 8, 2023

GlueJobOperator failing with Invalid type for parameter RoleName after updating provider version. #29960

Closed

2 tasks

jplauri mentioned this pull request Dec 13, 2023

Glue job not updating on iam_role_arn change GlueJobOperator/GlueJobHook #36203

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AWSGlueJobHook updates job configuration if it exists #27893

AWSGlueJobHook updates job configuration if it exists #27893

romibuzi commented Nov 24, 2022

boring-cyborg bot commented Nov 24, 2022

Taragolis Nov 24, 2022

romibuzi Nov 25, 2022

syedahsn Nov 24, 2022

romibuzi Nov 25, 2022

romibuzi commented Dec 1, 2022

Taragolis left a comment

vincbeck Dec 2, 2022

romibuzi Dec 5, 2022

vincbeck Dec 2, 2022

romibuzi Dec 5, 2022

vincbeck Dec 5, 2022

vincbeck Dec 2, 2022

romibuzi Dec 5, 2022

vincbeck Dec 5, 2022

vincbeck Dec 2, 2022

romibuzi Dec 5, 2022

vincbeck Dec 2, 2022

romibuzi commented Dec 6, 2022

boring-cyborg bot commented Dec 6, 2022

AWSGlueJobHook updates job configuration if it exists #27893

AWSGlueJobHook updates job configuration if it exists #27893

Conversation

romibuzi commented Nov 24, 2022

boring-cyborg bot commented Nov 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romibuzi commented Dec 1, 2022

Taragolis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romibuzi commented Dec 6, 2022

boring-cyborg bot commented Dec 6, 2022