Refactoring EmrClusterLink and add it for other AWS EMR Operators

[Taragolis]

Amazon provider at that moment has only one External link for EmrClusterLink.

The idea was taken from Google Cloud Provider.

Extend:

• Hook: Return region and aws partition from current connection/session
• Console link based on aws partition now
• Link contains region_name now

Add:

• AwsBaseLink for further Links (Batch, Glue, Cloudwatch, etc.)
• EmrClusterLink links to EmrAddStepsOperator, EmrModifyClusterOperator, EmrTerminateJobFlowOperator

[Taragolis]

cc: @ferruzzi if you have a time could you have a look on this

[ferruzzi]

It took me a minute to figure out what you were getting at, but it's clever and I like it. ((For anyone else who hasn't had a coffee yet, it's to add a website link back to the AWS Console in the Extra Links section of the Operators in the UI))

I wonder if we can figure out a way to work the operator_extra_links assignment into the AwsBaseOperator that you are working on as well, so we don't assign it each time. I'd need to think on it, but BaseOperator accepts a hook like EmrClusterHook. I wonder if we can work some shenanigans to use that classname to magically add the link if the matching link class exists. I'm not proposing that this gets held up for that, just a thought.

[ferruzzi]

@shubham22 Check this out! We were just talking about getting the Extra Links sections filled out for the operators when there was time!

[vincbeck]

Really nice work! Besides @ferruzzi comments, I dont have anything to say other than I like it :)

[shubham22]

@Taragolis - this is very nice and great design as well! We had in our backlog, but couldn't get to it as we are prioritizing System Test work. Thanks for taking the initiative.

[Taragolis]

I wonder if we can figure out a way to work the operator_extra_links assignment into the AwsBaseOperator that you are working on as well, so we don't assign it each time.

I think it is hardly possible because each operator could required each set of Links and not all of them related to Hook

Example:

• For EMR at that moment only link to cluster could be provided because there is no direct link to steps exists.
• For AWS Batch it could be set of links: Batch Job Detail, Latest Attempt logs in CloudWatch, Batch Job Definition, Batch Queue
• For Glue Job it could be set of links: Glue Job Detail (Glue ETL Studio), Glue Job Detail (Legacy), Glue Job, Filter to CloudWatch prefixes

[Taragolis]

We had in our backlog, but couldn't get to it as we are prioritizing System Test work.

no problem, anyway there is plenty amount of time to next provider release after 4.0.0

[o-nikolas]

Thanks for the contribution!

[ferruzzi]

I think it is hardly possible because each operator could required each set of Links and not all of them related to Hook

Maybe. But also, if BaseOperator created a Link set containing the link to the service's main console page, then that list could be extended in the operator if desired. That would save assigning it in every Operator and only need the declaration in Operators where more than the basic link are desired.

Either way, it's not a blocking suggestion, just a thought.

I'm also not sure what this current implementation is going to look like when you want to add a second/third/nth link.

[Taragolis]

Maybe. But also, if BaseOperator created a Link set containing the link to the service's main console page

The implementation of Aws Service link it straightforward

import attr
from airflow.providers.amazon.aws.links.base_aws import BaseAwsLink, BASE_AWS_CONSOLE_LINK


@attr.s(auto_attribs=True)
class AwsServiceLink(BaseAwsLink):
    name: str
    format_str: str
    key = "aws_service"

AwsServiceLink(
    name="AWS EMR Console Link",
    format_str=BASE_AWS_CONSOLE_LINK + "/elasticmapreduce/home?region={region_name}#"
)

The is two things which need to be check and solved

1. Each operator should call AwsServiceLink.persist in execute method of operator/sensor
2. Need to check is it fine to change operator_extra_links in runtime

I'm also not sure what this current implementation is going to look like when you want to add a second/third/nth link.

All operator links predefined in operator in that moment

In execute stage call persist method of link with appropriate keywords, which stored in XCom

[potiuk]

I think it's good as it is :). Any refactoring like that IMHO is never "complete" until you have 3-4 derived classes :). And we can always iterate if we find that we need to. I think you should merge that one and add few more links based on that and we will find out if more changes are needed WDYT @ferruzzi @Taragolis

[github-actions]

The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.

[ferruzzi]

Yeah, I'm good. Like I said, it's just something to consider, I didn't want to block the merge over that. Thanks for checking.