You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "vandonr-amz (via GitHub)" <gi...@apache.org> on 2023/07/05 23:08:08 UTC
[GitHub] [airflow] vandonr-amz opened a new pull request, #32386: Remove ability to specify arbitrary hook params in AWS RDS trigger
vandonr-amz opened a new pull request, #32386:
URL: https://github.com/apache/airflow/pull/32386
This is something that we are deprecating, and deferrable mode is a new feature, so by definition it cannot break any existing workflow. Thus, I don't think we need to support that obsolete feature in new code.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] vincbeck commented on a diff in pull request #32386: Remove ability to specify arbitrary hook params in AWS RDS trigger
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on code in PR #32386:
URL: https://github.com/apache/airflow/pull/32386#discussion_r1254783215
##########
airflow/providers/amazon/aws/operators/rds.py:
##########
@@ -47,14 +47,14 @@ def __init__(self, *args, aws_conn_id: str = "aws_conn_id", hook_params: dict |
if hook_params is not None:
warnings.warn(
"The parameter hook_params is deprecated and will be removed. "
+ "Note that it is also incompatible with deferrable mode. "
"If you were using it, please get in touch either on airflow slack, "
"or by opening a github issue on the project. "
"You can mention https://github.com/apache/airflow/pull/32352",
AirflowProviderDeprecationWarning,
stacklevel=3, # 2 is in the operator's init, 3 is in the user code creating the operator
)
- self.hook_params = hook_params or {}
Review Comment:
It feels duplicate to me ... But that's my developer opinion, I can understand, as a user, just passing a region as parameter is easier than creating a new connection. Maybe a question for @shubham22
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] shubham22 commented on a diff in pull request #32386: Remove ability to specify arbitrary hook params in AWS RDS trigger
Posted by "shubham22 (via GitHub)" <gi...@apache.org>.
shubham22 commented on code in PR #32386:
URL: https://github.com/apache/airflow/pull/32386#discussion_r1254822585
##########
airflow/providers/amazon/aws/operators/rds.py:
##########
@@ -47,14 +47,14 @@ def __init__(self, *args, aws_conn_id: str = "aws_conn_id", hook_params: dict |
if hook_params is not None:
warnings.warn(
"The parameter hook_params is deprecated and will be removed. "
+ "Note that it is also incompatible with deferrable mode. "
"If you were using it, please get in touch either on airflow slack, "
"or by opening a github issue on the project. "
"You can mention https://github.com/apache/airflow/pull/32352",
AirflowProviderDeprecationWarning,
stacklevel=3, # 2 is in the operator's init, 3 is in the user code creating the operator
)
- self.hook_params = hook_params or {}
Review Comment:
> just passing a region as parameter is easier than creating a new connection
I agree with this. I haven't had direct conversation on this with customers, but I would assume this is commonly done. Yes, many operators do not support it today and may be some users might request for them in the future. In any case, we shouldn't take it away if it is supported until and unless we think it is causing some other problems.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] vandonr-amz commented on pull request #32386: Remove ability to specify arbitrary hook params in AWS RDS trigger
Posted by "vandonr-amz (via GitHub)" <gi...@apache.org>.
vandonr-amz commented on PR #32386:
URL: https://github.com/apache/airflow/pull/32386#issuecomment-1624128610
Since RC2 has been bundled, this PR is not moot because it becomes a breaking change :/
I'll see what we can do given the new state of things.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] syedahsn commented on a diff in pull request #32386: Remove ability to specify arbitrary hook params in AWS RDS trigger
Posted by "syedahsn (via GitHub)" <gi...@apache.org>.
syedahsn commented on code in PR #32386:
URL: https://github.com/apache/airflow/pull/32386#discussion_r1254803865
##########
airflow/providers/amazon/aws/operators/rds.py:
##########
@@ -47,14 +47,14 @@ def __init__(self, *args, aws_conn_id: str = "aws_conn_id", hook_params: dict |
if hook_params is not None:
warnings.warn(
"The parameter hook_params is deprecated and will be removed. "
+ "Note that it is also incompatible with deferrable mode. "
"If you were using it, please get in touch either on airflow slack, "
"or by opening a github issue on the project. "
"You can mention https://github.com/apache/airflow/pull/32352",
AirflowProviderDeprecationWarning,
stacklevel=3, # 2 is in the operator's init, 3 is in the user code creating the operator
)
- self.hook_params = hook_params or {}
Review Comment:
>There is a bunch of operators that don't allow passing a region at all, like the sagemaker operators, DMS, etc.
I agree with you here, but in the interest of keeping things backwards compatible, if a particular operator allows passing region config, then we should continue that or if we want to stop supporting that, then we would need to go through the whole deprecating process.
At this point, I don't know how you want to proceed though :p
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] vincbeck commented on pull request #32386: Remove ability to specify arbitrary hook params in AWS RDS trigger
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on PR #32386:
URL: https://github.com/apache/airflow/pull/32386#issuecomment-1625898991
Amazon provider package is excluded from RC2, therefore we can merge it. See https://github.com/apache/airflow/issues/32389
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] syedahsn commented on a diff in pull request #32386: Remove ability to specify arbitrary hook params in AWS RDS trigger
Posted by "syedahsn (via GitHub)" <gi...@apache.org>.
syedahsn commented on code in PR #32386:
URL: https://github.com/apache/airflow/pull/32386#discussion_r1253766156
##########
airflow/providers/amazon/aws/operators/rds.py:
##########
@@ -47,14 +47,14 @@ def __init__(self, *args, aws_conn_id: str = "aws_conn_id", hook_params: dict |
if hook_params is not None:
warnings.warn(
"The parameter hook_params is deprecated and will be removed. "
+ "Note that it is also incompatible with deferrable mode. "
"If you were using it, please get in touch either on airflow slack, "
"or by opening a github issue on the project. "
"You can mention https://github.com/apache/airflow/pull/32352",
AirflowProviderDeprecationWarning,
stacklevel=3, # 2 is in the operator's init, 3 is in the user code creating the operator
)
- self.hook_params = hook_params or {}
Review Comment:
What happens if someone wants to pass something like `region_name` to the hook? This is a fairly common use case, and there is no reason to exclude that from being used in deferrable operators.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] vandonr-amz commented on a diff in pull request #32386: Remove ability to specify arbitrary hook params in AWS RDS trigger
Posted by "vandonr-amz (via GitHub)" <gi...@apache.org>.
vandonr-amz commented on code in PR #32386:
URL: https://github.com/apache/airflow/pull/32386#discussion_r1253791238
##########
airflow/providers/amazon/aws/operators/rds.py:
##########
@@ -47,14 +47,14 @@ def __init__(self, *args, aws_conn_id: str = "aws_conn_id", hook_params: dict |
if hook_params is not None:
warnings.warn(
"The parameter hook_params is deprecated and will be removed. "
+ "Note that it is also incompatible with deferrable mode. "
"If you were using it, please get in touch either on airflow slack, "
"or by opening a github issue on the project. "
"You can mention https://github.com/apache/airflow/pull/32352",
AirflowProviderDeprecationWarning,
stacklevel=3, # 2 is in the operator's init, 3 is in the user code creating the operator
)
- self.hook_params = hook_params or {}
Review Comment:
pushed a change to that effect.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] vandonr-amz commented on a diff in pull request #32386: Remove ability to specify arbitrary hook params in AWS RDS trigger
Posted by "vandonr-amz (via GitHub)" <gi...@apache.org>.
vandonr-amz commented on code in PR #32386:
URL: https://github.com/apache/airflow/pull/32386#discussion_r1253783399
##########
airflow/providers/amazon/aws/operators/rds.py:
##########
@@ -47,14 +47,14 @@ def __init__(self, *args, aws_conn_id: str = "aws_conn_id", hook_params: dict |
if hook_params is not None:
warnings.warn(
"The parameter hook_params is deprecated and will be removed. "
+ "Note that it is also incompatible with deferrable mode. "
"If you were using it, please get in touch either on airflow slack, "
"or by opening a github issue on the project. "
"You can mention https://github.com/apache/airflow/pull/32352",
AirflowProviderDeprecationWarning,
stacklevel=3, # 2 is in the operator's init, 3 is in the user code creating the operator
)
- self.hook_params = hook_params or {}
Review Comment:
There is a bunch of operators that don't allow passing a region at all, like the sagemaker operators, DMS, etc.
If we want users to be able to pass a region, I think we should add that as an explicit parameter.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] vincbeck merged pull request #32386: Remove ability to specify arbitrary hook params in AWS RDS trigger
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck merged PR #32386:
URL: https://github.com/apache/airflow/pull/32386
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ferruzzi commented on pull request #32386: Remove ability to specify arbitrary hook params in AWS RDS trigger
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on PR #32386:
URL: https://github.com/apache/airflow/pull/32386#issuecomment-1622671671
@syedahsn you just added those lines last week, are you good with this?
Assuming he is cool with it, the code itself LGTM
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] vandonr-amz commented on a diff in pull request #32386: Remove ability to specify arbitrary hook params in AWS RDS trigger
Posted by "vandonr-amz (via GitHub)" <gi...@apache.org>.
vandonr-amz commented on code in PR #32386:
URL: https://github.com/apache/airflow/pull/32386#discussion_r1254779635
##########
airflow/providers/amazon/aws/operators/rds.py:
##########
@@ -47,14 +47,14 @@ def __init__(self, *args, aws_conn_id: str = "aws_conn_id", hook_params: dict |
if hook_params is not None:
warnings.warn(
"The parameter hook_params is deprecated and will be removed. "
+ "Note that it is also incompatible with deferrable mode. "
"If you were using it, please get in touch either on airflow slack, "
"or by opening a github issue on the project. "
"You can mention https://github.com/apache/airflow/pull/32352",
AirflowProviderDeprecationWarning,
stacklevel=3, # 2 is in the operator's init, 3 is in the user code creating the operator
)
- self.hook_params = hook_params or {}
Review Comment:
I don't exactly know how that works in practice... I suppose you'd have a "main" region where you do the heavy lifting (sagemaker stuff for instance), but you'd have DBs in various regions, and you'd like to be able to hit those without having to change too much stuff ?
Are you saying that the region param should be obsoleted everywhere in the AWS provider in favor of several connection ids ?
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] vincbeck commented on a diff in pull request #32386: Remove ability to specify arbitrary hook params in AWS RDS trigger
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on code in PR #32386:
URL: https://github.com/apache/airflow/pull/32386#discussion_r1254543116
##########
airflow/providers/amazon/aws/operators/rds.py:
##########
@@ -47,14 +47,14 @@ def __init__(self, *args, aws_conn_id: str = "aws_conn_id", hook_params: dict |
if hook_params is not None:
warnings.warn(
"The parameter hook_params is deprecated and will be removed. "
+ "Note that it is also incompatible with deferrable mode. "
"If you were using it, please get in touch either on airflow slack, "
"or by opening a github issue on the project. "
"You can mention https://github.com/apache/airflow/pull/32352",
AirflowProviderDeprecationWarning,
stacklevel=3, # 2 is in the operator's init, 3 is in the user code creating the operator
)
- self.hook_params = hook_params or {}
Review Comment:
This is something we might want to think about actually. In which case a user might want to specify another region? Regions are set as part of connections. That means, a user set `regionA` in its connection, hence targeting this regions for all AWS calls but would want to target another region for a specific operator? I think that can be done by using another connection then (and then using the param `aws_conn_id`). WDYT?
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] eladkal commented on pull request #32386: Remove ability to specify arbitrary hook params in AWS RDS trigger
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on PR #32386:
URL: https://github.com/apache/airflow/pull/32386#issuecomment-1624157543
> Since RC2 has been bundled, this PR is now moot because it becomes a breaking change :/ I'll see what we can do given the new state of things.
For the moment I suggest to wait till vote is over.
In case we will have RC3
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org