Re: [PR] Add error_statuses verification in RdsExportTaskExistenceSensor [airflow]
vargacypher closed pull request #46593: Add error_statuses verification in RdsExportTaskExistenceSensor URL: https://github.com/apache/airflow/pull/46593 -- 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
Re: [PR] Add error_statuses verification in RdsExportTaskExistenceSensor [airflow]
manya5 commented on PR #46593: URL: https://github.com/apache/airflow/pull/46593#issuecomment-2650010829 "Hi, @vargacypher could I help you with something, since I'm new to GSOC and Airflow and want to start it with -- 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
Re: [PR] Add error_statuses verification in RdsExportTaskExistenceSensor [airflow]
vargacypher closed pull request #46593: Add error_statuses verification in RdsExportTaskExistenceSensor URL: https://github.com/apache/airflow/pull/46593 -- 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
Re: [PR] Add error_statuses verification in RdsExportTaskExistenceSensor [airflow]
vargacypher commented on PR #46593: URL: https://github.com/apache/airflow/pull/46593#issuecomment-2646334353 Sorry for the wrong requested review guy's -- 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
Re: [PR] Add error_statuses verification in RdsExportTaskExistenceSensor [airflow]
vargacypher commented on code in PR #46593:
URL: https://github.com/apache/airflow/pull/46593#discussion_r1948081471
##
providers/src/airflow/providers/amazon/aws/sensors/rds.py:
##
@@ -129,13 +128,19 @@ def __init__(
"canceling",
"canceled",
]
+self.error_statuses = error_statuses or ["failed"]
def poke(self, context: Context):
self.log.info(
"Poking for statuses : %s\nfor export task %s",
self.target_statuses, self.export_task_identifier
)
try:
state =
self.hook.get_export_task_state(self.export_task_identifier)
+if state in self.error_statuses:
+raise AirflowFailException(
+f"Export task {self.export_task_identifier} failed with
status {state}"
Review Comment:
We're in doubt in wich was the best exception to raise and stop the sensor,
what do u think it will be better ?
We could implement a custom one.
--
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
Re: [PR] Add error_statuses verification in RdsExportTaskExistenceSensor [airflow]
vargacypher commented on code in PR #46593:
URL: https://github.com/apache/airflow/pull/46593#discussion_r1948081471
##
providers/src/airflow/providers/amazon/aws/sensors/rds.py:
##
@@ -129,13 +128,19 @@ def __init__(
"canceling",
"canceled",
]
+self.error_statuses = error_statuses or ["failed"]
def poke(self, context: Context):
self.log.info(
"Poking for statuses : %s\nfor export task %s",
self.target_statuses, self.export_task_identifier
)
try:
state =
self.hook.get_export_task_state(self.export_task_identifier)
+if state in self.error_statuses:
+raise AirflowFailException(
+f"Export task {self.export_task_identifier} failed with
status {state}"
Review Comment:
We're in doubt in wich was the best exception to raise and stop the sensor,
what do u think it will be better ?
--
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
Re: [PR] Add error_statuses verification in RdsExportTaskExistenceSensor [airflow]
eladkal commented on code in PR #46593:
URL: https://github.com/apache/airflow/pull/46593#discussion_r1948031869
##
providers/src/airflow/providers/amazon/aws/sensors/rds.py:
##
@@ -129,13 +128,19 @@ def __init__(
"canceling",
"canceled",
]
+self.error_statuses = error_statuses or ["failed"]
def poke(self, context: Context):
self.log.info(
"Poking for statuses : %s\nfor export task %s",
self.target_statuses, self.export_task_identifier
)
try:
state =
self.hook.get_export_task_state(self.export_task_identifier)
+if state in self.error_statuses:
+raise AirflowFailException(
+f"Export task {self.export_task_identifier} failed with
status {state}"
Review Comment:
Why `AirflowFailException`?
--
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
Re: [PR] Add error_statuses verification in RdsExportTaskExistenceSensor [airflow]
vargacypher commented on code in PR #46593: URL: https://github.com/apache/airflow/pull/46593#discussion_r1948013828 ## providers/tests/amazon/aws/sensors/test_rds.py: ## @@ -223,6 +238,19 @@ def test_export_task_poke_false(self): ) assert not op.poke(None) +@mock_aws +def test_error_statuses(self): +# Simulate an error condition +_start_export_task_with_error(self.hook) +op = RdsExportTaskExistenceSensor( +task_id="export_task_error", +export_task_identifier=EXPORT_TASK_NAME, +aws_conn_id=AWS_CONN, +dag=self.dag, +) + +assert "failed" in op.error_statuses Review Comment: Adding it -- 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
Re: [PR] Add error_statuses verification in RdsExportTaskExistenceSensor [airflow]
vargacypher commented on code in PR #46593:
URL: https://github.com/apache/airflow/pull/46593#discussion_r1948013182
##
providers/src/airflow/providers/amazon/aws/sensors/rds.py:
##
@@ -104,18 +104,17 @@ class RdsExportTaskExistenceSensor(RdsBaseSensor):
:param export_task_identifier: A unique identifier for the snapshot export
task.
:param target_statuses: Target status of export task
+:param error_statuses: Target error status of export task to fail the
sensor
"""
-template_fields: Sequence[str] = (
-"export_task_identifier",
-"target_statuses",
-)
+template_fields: Sequence[str] = ("export_task_identifier",
"target_statuses", "error_statuses")
Review Comment:
ruff format is changing it.
--
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
Re: [PR] Add error_statuses verification in RdsExportTaskExistenceSensor [airflow]
tgmti commented on code in PR #46593: URL: https://github.com/apache/airflow/pull/46593#discussion_r1948012619 ## providers/tests/amazon/aws/sensors/test_rds.py: ## @@ -223,6 +238,19 @@ def test_export_task_poke_false(self): ) assert not op.poke(None) +@mock_aws +def test_error_statuses(self): +# Simulate an error condition +_start_export_task_with_error(self.hook) +op = RdsExportTaskExistenceSensor( +task_id="export_task_error", +export_task_identifier=EXPORT_TASK_NAME, +aws_conn_id=AWS_CONN, +dag=self.dag, +) + +assert "failed" in op.error_statuses Review Comment: this shouldn't test raise of AirflowFailException? -- 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
Re: [PR] Add error_statuses verification in RdsExportTaskExistenceSensor [airflow]
tgmti commented on code in PR #46593:
URL: https://github.com/apache/airflow/pull/46593#discussion_r1948012084
##
providers/src/airflow/providers/amazon/aws/sensors/rds.py:
##
@@ -104,18 +104,17 @@ class RdsExportTaskExistenceSensor(RdsBaseSensor):
:param export_task_identifier: A unique identifier for the snapshot export
task.
:param target_statuses: Target status of export task
+:param error_statuses: Target error status of export task to fail the
sensor
"""
-template_fields: Sequence[str] = (
-"export_task_identifier",
-"target_statuses",
-)
+template_fields: Sequence[str] = ("export_task_identifier",
"target_statuses", "error_statuses")
Review Comment:
to keep the same format
```suggestion
template_fields: Sequence[str] = (
"export_task_identifier",
"target_statuses",
"error_statuses",
)
```
--
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
Re: [PR] Add error_statuses verification in RdsExportTaskExistenceSensor [airflow]
vargacypher commented on PR #46593: URL: https://github.com/apache/airflow/pull/46593#issuecomment-2646063540 I'll fix the **lambda** call in test. -- 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
