rewrite polling code for appflow hook #28869
Conversation
boring-cyborg
bot
added
area:providers
provider:amazon
| # previous code had a wait between update and run without explaining why. | ||
| # since I don't have a way to actually test this behavior, | ||
| # I'm reproducing it out of fear of breaking workflows. | ||
| # It might be unnecessary. |
There was a problem hiding this comment.
If I recall correctly, the updates are not propagated atomically. So this sleep was to avoid runs using the previous configuration.
There was a problem hiding this comment.
checked it with the team, and it's true for on-demand flows (see https://docs.aws.amazon.com/appflow/latest/userguide/flow-triggers.html for a desc of the different types of flows)
I updated the comment accordingly.
Unfortunately, we don't have infra setup to be able to run the appflow system test (because it depends on external sources), so it's a bit hard for me to test it.
|
Great improvements @vandonr-amz, thank you! Just one point, I recommend testing it against the real service. |
|
@Taragolis @igorborgest are you happy with the state of this PR or would you like to see any other changes/testing? |
|
@o-nikolas Oh, I really do not use Appflow. I've just ping Igor, because remember (and git blame told me) that he initially implement. But let me have a look. |
|
|
||
| if last_exec_details["mostRecentExecutionStatus"] == "Error": | ||
| raise Exception(f"Flow error:\n{json.dumps(response_desc, default=str)}") | ||
| if wait_for_completion: |
There was a problem hiding this comment.
Instead of having this custom logic, would not it be possible to use a custom waiter as in #29822?
There was a problem hiding this comment.
ok, but do you think it should be done as part of this PR ? How about I push the conversion to waiter as a new PR ?
There was a problem hiding this comment.
To me it should be done in this PR. It is basically the same problem with 2 different implementations, let's aim directly the desire implementation
There was a problem hiding this comment.
I definitely agree that this should be a waiter and may as well be done now.
There was a problem hiding this comment.
ok, but after taking a look at it, waiter configurations don't allow implementing the logic needed here.
Because the API doesn't allow querying status by run ID, we have to employ other means. The old logic was somewhat flawed but still checked the timestamp to make sure we weren't picking up the previous run. The new logic relies on the run ID. Both of those behavior are not possible with regular waiters, so I wouldn't block this PR on this.
There was a problem hiding this comment.
We talked offline and it does look like the AppFlow waiters are a non-trivial change. They handle their waiters completely differently than other services. I still think a custom-waiters-first approach is
the best approach, but in this case I'm willing to approve this as is.
There was a problem hiding this comment.
We talked offline and it does look like the AppFlow waiters are a non-trivial change. They handle their waiters completely differently than other services. I still think a custom-waiters-first approach is the best approach, but in this case I'm willing to approve this as is.
Edit to clarify: "custom-waiters-first approach" meaning we should prefer them when reasonably possible. In this case it's a LOT more work for marginal gain. "I don't feel like updating the json file" isn't IMHO a valid reason to skip using a custom waiter design.
6 participants
Existing code had several problems imho, like
I also refactored some duplicated code and added a
wait_for_completionparameter to skip the wait entirely if needed.