Remove ability to specify arbitrary hook params in AWS RDS trigger #32386
Conversation
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.
boring-cyborg
bot
added
area:providers
provider:amazon
|
@syedahsn you just added those lines last week, are you good with this? Assuming he is cool with it, the code itself LGTM |
| 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 {} |
There was a problem hiding this 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.
There was a problem hiding this 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.
There was a problem hiding this comment.
pushed a change to that effect.
There was a problem hiding this 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?
There was a problem hiding this 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 ?
There was a problem hiding this 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
There was a problem hiding this 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
There was a problem hiding this 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.
|
Since RC2 has been bundled, this PR is now moot because it becomes a breaking change :/ |
For the moment I suggest to wait till vote is over. |
|
Amazon provider package is excluded from RC2, therefore we can merge it. See #32389 |
6 participants
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.
Also added a region_name param to the base rds operator.