Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merge sync and async operator #40

Merged
merged 5 commits into from
Aug 7, 2023
Merged

Merge sync and async operator #40

merged 5 commits into from
Aug 7, 2023

Conversation

pankajastro
Copy link
Contributor

@pankajastro pankajastro commented Jul 27, 2023

closes: #35

  • deprecate FivetranSensorAsync and FivetranOperatorAsync
  • Add deferrable param in FivetranSensor and FivetranOperator

@pankajastro pankajastro marked this pull request as ready for review July 27, 2023 21:01
Copy link
Contributor

@phanikumv phanikumv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The repo name is airflow-provider-fivetran-async and we are deprecating the async operator. I think we should merge this the other way, i.e., having FivetranOperatorAsync and have deferrable as True by default. Having it as False, should invoke the sync part of the operator

@pankajastro
Copy link
Contributor Author

The repo name is airflow-provider-fivetran-async and we are deprecating the async operator. I think we should merge this the other way, i.e., having FivetranOperatorAsync and have deferrable as True by default. Having it as False, should invoke the sync part of the operator

The current default value of the deferrable param is True. I decided to keep FivetranOperator without the suffix Async because I was thinking of having the suffix Async in the operator/sensor name and the deferrable param looks not in sync. But happy to change it if you think otherwise makes more sense.

fivetran_provider_async/operators.py Outdated Show resolved Hide resolved
fivetran_provider_async/operators.py Outdated Show resolved Hide resolved
fivetran_provider_async/operators.py Outdated Show resolved Hide resolved
fivetran_provider_async/operators.py Outdated Show resolved Hide resolved
fivetran_provider_async/operators.py Show resolved Hide resolved
fivetran_provider_async/sensors.py Outdated Show resolved Hide resolved
fivetran_provider_async/sensors.py Outdated Show resolved Hide resolved
fivetran_provider_async/sensors.py Show resolved Hide resolved
fivetran_provider_async/sensors.py Show resolved Hide resolved
@pankajastro pankajastro force-pushed the merge_sync_async_op branch 2 times, most recently from 9221e83 to 3b0de44 Compare August 4, 2023 11:16
@phanikumv phanikumv dismissed their stale review August 4, 2023 12:36

agree with the point discussed on sync vs async merge

`FivetranSensorAsync` allows you to monitor a Fivetran sync job for completion before running downstream processes.
`FivetranOperatorAsync` submits a Fivetran sync job and polls for its status on the triggerer.
`FivetranSensor` allows you to monitor a Fivetran sync job for completion before running downstream processes.
`FivetranOperator` submits a Fivetran sync job and polls for its status on the triggerer.
Copy link
Contributor

@phanikumv phanikumv Aug 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
`FivetranOperator` submits a Fivetran sync job and polls for its status on the triggerer.
`FivetranOperator` submits a Fivetran sync job and polls for it's status on the triggerer.When deferrable param is set to False, it would not release the Airflow worker slot and waits for the job to complete.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@phanikumv the FivetranOperator does not wait for the job to complete. It only submits and then we use a sensor to monitor for completion

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah right sorry , just confused with OSS operators :)

@phanikumv phanikumv merged commit 362337a into main Aug 7, 2023
5 checks passed
@phanikumv phanikumv deleted the merge_sync_async_op branch August 7, 2023 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Merge sync and async operator sensor
3 participants