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

Add FivetranOperatorAsync hook and trigger to handle reschedule changes #25

Merged
merged 12 commits into from
Jun 7, 2023

Conversation

sunank200
Copy link
Contributor

@sunank200 sunank200 commented May 25, 2023

  • Add FivetranOperatorAsync to handle reschedule mode
  • When connector has schedule_type manual and sync_status reschedule, connector will hang as Fivetran expects manual restart. This pr automatically (or optionally manually) handles that restart.
  • Add tests
    closes: FivetranOperatorAsync waits forever in case of reschedule #21

@sunank200 sunank200 marked this pull request as draft May 25, 2023 07:06
@sunank200 sunank200 marked this pull request as ready for review May 31, 2023 20:29
Copy link
Contributor

@pankajastro pankajastro left a comment

Choose a reason for hiding this comment

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

shall we add this param here too while initialising trigger

fivetran_provider_async/hooks.py Outdated Show resolved Hide resolved
fivetran_provider_async/hooks.py Outdated Show resolved Hide resolved
fivetran_provider_async/hooks.py Outdated Show resolved Hide resolved
fivetran_provider_async/hooks.py Outdated Show resolved Hide resolved
fivetran_provider_async/hooks.py Outdated Show resolved Hide resolved
fivetran_provider_async/hooks.py Show resolved Hide resolved
fivetran_provider_async/hooks.py Show resolved Hide resolved
@sunank200
Copy link
Contributor Author

shall we add this param here too while initialising trigger

Added the change

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.

Dont we need tests to be added for operator and sensor for reschedule change?

@sunank200
Copy link
Contributor Author

Dont we need tests to be added for operator and sensor for reschedule change?

I have added the relevant tests in hook and triggerer. I can add tests in for sensor and operator as well

Copy link
Contributor

@pankajkoti pankajkoti left a comment

Choose a reason for hiding this comment

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

I have the below questions:

  1. Does rescheduled_for exists & applies for connector_details["schedule_type"] == "manual" in FiveTran?
  2. Were we able to test the issue fix E2E with a DAG mentioned in the PR description wrt
    When connector has schedule_type manual and sync_status reschedule, connector will hang as Fivetran expects manual restart. This pr automatically (or optionally manually) handles that restart.?

If the answer is yes to both the questions, then the changes look good to me.

fivetran_provider_async/hooks.py Outdated Show resolved Hide resolved
@sunank200
Copy link
Contributor Author

I have the below questions:

  1. Does rescheduled_for exists & applies for connector_details["schedule_type"] == "manual" in FiveTran?
    @pankajkoti
  1. Yes rescheduled_for and `connector_details["schedule_type"] == "manual" exists in Fivetran as per the API documentation as well: https://fivetran.com/docs/rest-api/connectors#fields

@sunank200
Copy link
Contributor Author

sunank200 commented Jun 7, 2023

I have the below questions:

  1. Were we able to test the issue fix E2E with a DAG mentioned in the PR description wrt
    When connector has schedule_type manual and sync_status reschedule, connector will hang as Fivetran expects manual restart. This pr automatically (or optionally manually) handles that restart.?

If the answer is yes to both the questions, then the changes look good to me.

I have tested it locally using the following task:

fivetran_async_op = FivetranOperatorAsync(
        task_id="fivetran_async_op",
        connector_id="rolled_praising",
        fivetran_conn_id="fivetran_default",
        reschedule_wait_time=60,
        schedule_type="manual"
    )

In this PR, we automatically handle the case of restart of the sync.

@sunank200
Copy link
Contributor Author

Update the tests

@phanikumv I have updated the test for various scenarios: 4539e03

@sunank200 sunank200 merged commit 0fe10fc into main Jun 7, 2023
@sunank200 sunank200 deleted the fivetran-reschedule branch June 7, 2023 15:41
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.

FivetranOperatorAsync waits forever in case of reschedule
5 participants