AchimGaedkeLynker commented on code in PR #36828:
URL: https://github.com/apache/airflow/pull/36828#discussion_r1456505130
##########
airflow/providers/amazon/aws/operators/ec2.py:
##########
@@ -183,22 +183,36 @@ def execute(self, context: Context):
MaxCount=self.max_count,
**self.config,
)["Instances"]
- instance_ids = []
- for instance in instances:
- instance_ids.append(instance["InstanceId"])
- self.log.info("Created EC2 instance %s", instance["InstanceId"])
+
+ instance_ids = self._on_kill_instance_ids = [instance["InstanceId"]
for instance in instances]
Review Comment:
This type of clean-up in case of an error is very similar to the purpose of
[context/with statements in
python](https://realpython.com/python-with-statement/#the-with-statement-approach)
`on_kill` is called outside the normal execution sequence as part of the
signal handling for `SIGTERM` - hence the extra variable only for that purpose.
Yes, one could call it `self.instance_ids` - I actually considered it -
this depends on the lifecycle of the `TaskInstance` object, call order of the
methods. So the list of instances to clean up should be cleared when `XCom` has
communicated the result, I assumed that happens last. I named the variable very
purpose specific.
Would you think it would be necessary to delete the `_on_kill_instance_ids`,
e.g. as part of the `post_execute` method?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
