Add DMS Serverless Operators #43988
Add DMS Serverless Operators #43988
Conversation
boring-cyborg
bot
added
area:providers
kind:documentation
provider:amazon-aws
| @@ -219,3 +224,158 @@ def wait_for_task_status(self, replication_task_arn: str, status: DmsTaskWaiterS | |||
| ], | |||
| WithoutSettings=True, | |||
| ) | |||
|
|
|||
There was a problem hiding this comment.
Overall I am wondering if it is worth creating all these methods, they are all just wrapper around boto3 apis. You added some error management around it but I am just questioning if it is worth creating new methods for that.
There was a problem hiding this comment.
The describe_* methods get used in a few places, so I think those should be in the hook to avoid duplication. I'll look to consolidate some of the simpler ones directly into the Operator.
There was a problem hiding this comment.
I think some below are quite simple, but others are decent number of lines that's nice to abstract out of the Operators (start and create for example). So I'm on the fence here, I could see both ways, but might lean on keeping it how it is since you've already spent time writing and testing the code as it is?
| result = self.hook.describe_replications( | ||
| filters=[{"Name": "replication-config-arn", "Values": [self.replication_config_arn]}] | ||
| ) | ||
|
|
||
| try: | ||
| current_status = result[0].get("Status", "") | ||
| except Exception as ex: | ||
| self.log.error("Error while getting replication status: %s. Unable to start replication", str(ex)) | ||
| raise ex |
There was a problem hiding this comment.
Same here, I dont think it is necessary
| if self.deferrable: | ||
| self.log.info("Deferring until deprovisioning completes.") | ||
| self.defer( | ||
| trigger=DmsReplicationDeprovisionedTrigger( | ||
| replication_config_arn=self.replication_config_arn, | ||
| waiter_delay=self.waiter_delay, | ||
| waiter_max_attempts=self.waiter_max_attempts, | ||
| aws_conn_id=self.aws_conn_id, | ||
| ), | ||
| method_name="retry_execution", | ||
| ) |
There was a problem hiding this comment.
This comment also apply for the above operator. I am wondering if you should not split this operator in two. One sensor would wait for the deprovisioning to complete, and this operator would fail if the status is not complete before actually doing it. I just think this operator does a lot and the user does not necessarily know that the operator could potentially wait for some time (with a cost) even though wait_for_completion is False
There was a problem hiding this comment.
Good point. The DMS docs weren't clear that a replication must be stopped (or failed) AND deprovisioned, so I was trying to cover both cases to simplify the deletion process. System testing pointed this out. It does add a bit to the operator, but the wait time would be the same in the Sensor + Operator combo. I see meeting the pre-requisite for deleting different than wait_for_completion of the actual delete. Open to exploring creating a sensor and would like to hear what others think.
| @@ -219,3 +224,158 @@ def wait_for_task_status(self, replication_task_arn: str, status: DmsTaskWaiterS | |||
| ], | |||
| WithoutSettings=True, | |||
| ) | |||
|
|
|||
There was a problem hiding this comment.
I think some below are quite simple, but others are decent number of lines that's nice to abstract out of the Operators (start and create for example). So I'm on the fence here, I could see both ways, but might lean on keeping it how it is since you've already spent time writing and testing the code as it is?
Co-authored-by: Niko Oliveira <[email protected]>
Co-authored-by: Niko Oliveira <[email protected]>
Co-authored-by: Niko Oliveira <[email protected]>
Adding operators, waiters, and triggers to support AWS DMS Serverless replications.
closes #39954