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

Add RdsDbSensor to amazon provider package #26003

Merged
merged 17 commits into from
Sep 9, 2022

Conversation

hankehly
Copy link
Contributor

@hankehly hankehly commented Aug 27, 2022

Related: #25952

Summary

This PR adds the RdsDbSensor to the amazon provider package. It waits for an RDS instance or cluster to reach one (or more) of the DB instance states described here.

Todo

  • Add RdsDbSensor
  • Add/run unit tests
breeze shell
pytest tests/providers/amazon/aws/sensors/test_rds.py::TestRdsDbSensor
  • Add/run system tests
breeze shell
export AWS_ACCESS_KEY=***
export AWS_SECRET_ACCESS_KEY=***
airflow dags test -S tests/system/providers/amazon/aws/rds/example_rds_instance.py example_rds_instance 2022-08-01
  • Update documentation

@@ -38,10 +38,10 @@ def __init__(self, *args, aws_conn_id: str = "aws_conn_id", hook_params: Optiona
hook_params = hook_params or {}
self.hook = RdsHook(aws_conn_id=aws_conn_id, **hook_params)
self.target_statuses: List[str] = []
self.check_status_field = "Status"
Copy link
Contributor Author

@hankehly hankehly Aug 27, 2022

Choose a reason for hiding this comment

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

Current sensors rely on the same botocore response structure. The response to describe_db_instances uses a different field name to describe the state of the requested resource. I chose to define the field name as an instance variable because..

  • no need to update other sensor classes
  • no need to modify method signatures
  • minimal code changes
  • no extra conditional statements
  • it makes sense to define "sensor specific metadata" on the sensor class/instance

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this variable could be in _check_item method.
Based on item_type you could identify the status field

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, but that requires adding an if-else conditional statement. Is the current approach OK with you?

Copy link
Contributor

@kazanzhy kazanzhy Sep 5, 2022

Choose a reason for hiding this comment

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

I think it is OK to add one more if-else.
Actually, I meant something like:

    def _check_item(self, item_type: str, item_name: str) -> bool:
        """Get certain item from `_describe_item()` and check its status"""
        if item_type == 'db_instance':
            status_field = 'DBInstanceStatus'
        else:
            status_field = 'Status'

        try:
            items = self._describe_item(item_type, item_name)
        except ClientError:
            return False
        else:
            return bool(items) and any(
                map(lambda status: items[0][status_field].lower() == status, self.target_statuses)
            )

Seems only DBInstance has this issue with the Status field.
WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By adding if/else logic to the base class, we're making it "aware" of the details of the subclass. I think a better approach is to let the subclass tell us what it needs.

I checked the botocore documentation and it looks like "DBInstanceStatus" and "Status" are the only 2 options, so I'll go ahead and make the change to finish up this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed in afefe36

:dedent: 4
:start-after: [START howto_sensor_rds_instance]
:end-before: [END howto_sensor_rds_instance]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

rds-instance-sensor

Copy link
Contributor Author

@hankehly hankehly Sep 5, 2022

Choose a reason for hiding this comment

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

2022/09/05 update

Screen Shot 2022-09-05 at 13 13 54

@hankehly hankehly marked this pull request as ready for review August 27, 2022 07:09
@hankehly hankehly requested a review from mik-laj as a code owner August 27, 2022 07:09
@hankehly
Copy link
Contributor Author

@o-nikolas @vincbeck @ferruzzi @potiuk
Please review this at your earliest convenience.

Copy link
Contributor

@vincbeck vincbeck left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@ferruzzi ferruzzi left a comment

Choose a reason for hiding this comment

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

Left a couple comments; non-blocking.

airflow/providers/amazon/aws/sensors/rds.py Outdated Show resolved Hide resolved
@hankehly hankehly requested review from ferruzzi and removed request for mik-laj August 30, 2022 00:26
@ferruzzi
Copy link
Contributor

My concerns have been addressed, thanks. LGTM

@hankehly
Copy link
Contributor Author

hankehly commented Sep 1, 2022

@o-nikolas @vincbeck @ferruzzi @potiuk
Could someone please merge this PR, or let me know what else needs to change

@o-nikolas
Copy link
Contributor

@o-nikolas @vincbeck @ferruzzi @potiuk Could someone please merge this PR, or let me know what else needs to change

Unfortunately none of us tagged above have the powers to merge code except for @potiuk

@hankehly hankehly requested review from kazanzhy and removed request for ferruzzi September 5, 2022 00:06
@hankehly hankehly marked this pull request as ready for review September 5, 2022 07:28
@hankehly hankehly marked this pull request as draft September 5, 2022 07:33
Comment on lines +70 to +72
return bool(items) and any(
map(lambda status: items[0][status_field].lower() == status, self.target_statuses)
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, the return could be modified from map to the generator if you wish.

Suggested change
return bool(items) and any(
map(lambda status: items[0][status_field].lower() == status, self.target_statuses)
)
return bool(items) and any(items[0][status_field].lower() == status for status in self.target_statuses)

For me, both variants are well readable but maybe this one is better. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestions. I'd like to make these updates in a separate PR.
The formatting is the result of black (line too long)

else:
raise AirflowException(f"Method for {item_type} is not implemented")

def _check_item(self, item_type: str, item_name: str) -> bool:
"""Get certain item from `_describe_item()` and check its status"""
if item_type == "db_instance":
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update 2022/09/06
#26003 (comment)

@hankehly hankehly marked this pull request as ready for review September 6, 2022 05:12
@hankehly hankehly requested review from ferruzzi and kazanzhy and removed request for ferruzzi and kazanzhy September 6, 2022 05:12
@hankehly
Copy link
Contributor Author

hankehly commented Sep 6, 2022

@o-nikolas @ferruzzi @vincbeck @kazanzhy
I renamed RdsInstanceSensor to RdsDbSensor and added the db_type parameter to match other sensor classes.

I left comments where the code changed.
Please re-OK the changes at your earliest convenience.

Copy link
Contributor

@ferruzzi ferruzzi left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the updates.

@potiuk potiuk merged commit a45ab47 into apache:main Sep 9, 2022
@hankehly hankehly deleted the issue-25952-add-rds-instance-sensor branch September 9, 2022 03:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants