-
Notifications
You must be signed in to change notification settings - Fork 15.8k
Add extra links for google dataproc #10343
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
Conversation
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/master/CONTRIBUTING.rst)
|
@mik-laj However, I've the following questions.
|
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 suppose you did this because of adding xcom_push
in execute
, right?
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.
Yes.
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.
Have added a mock task instance for testing the xcom push. This change is regarding the operator extra links. Please take a look at the updated tests.
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.
We should also add the console link to
airflow/airflow/serialization/serialized_objects.py
Lines 44 to 49 in 558be73
BUILTIN_OPERATOR_EXTRA_LINKS: List[str] = [ | |
"airflow.providers.google.cloud.operators.bigquery.BigQueryConsoleLink", | |
"airflow.providers.google.cloud.operators.bigquery.BigQueryConsoleIndexableLink", | |
"airflow.providers.google.cloud.operators.mlengine.AIPlatformConsoleLink", | |
"airflow.providers.qubole.operators.qubole.QDSLink" | |
] |
And as mentioned in a comment - we should add the link in DataprocSubmitJobOperator
as other ops are deprecated. So, we may even consider to add this only to this one operator 😉
@turbaszek I've added to those other operator as well already. |
The link is not being redirected to the cluster URL if there is no detail of the job in the system.
Implemented on both the operators. For subclasses of DataprocJobBaseOperators, I've added tests only for DataprocSubmitSparkJob operator as the other operators are anyway deprecated. Please have a look at the above comments by @turbaszek regarding 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.
LGTM! 🚀
@yesemsanthoshkumar can you please rebase? We started using black on providers package 👍 |
Nice to see this as part of #10014 |
5e772cb
to
6364da7
Compare
Tested both links. This rocks! Thanks @yesemsanthoshkumar 🚀 |
Add extra links for google cloud operators - Dataproc
How it is implemented
Note: I haven't worked with dataproc workflows so I haven't implemented links for WorkFlow related operators.
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code change, 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 UPDATING.md.