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

[ISSUE #15628] apply lookback window on earliest datetime between sta… #20156

Merged

Conversation

maxi297
Copy link
Contributor

@maxi297 maxi297 commented Dec 6, 2022

…rt and cursor

What

This change addresses issue #15628 . The lookback period should be applied to whatever is the most recent datetime between the start and the cursor defined by the state.

How

Applying the lookback window on max(start_datetime, cursor_datetime) instead of the start_datetime

🚨 User Impact 🚨

There should not be any breaking changes. However starting from this version, we will pull more data if there is a lookback window defined. So assuming a source with a state including the cursor field and a lookback window:

  • Before: incremental sync would start from the cursor field value defined in the state
  • After: incremental sync will start from the cursor field value defined in the state minus the lookback window

@maxi297 maxi297 requested a review from a team as a code owner December 6, 2022 19:49
@octavia-squidington-iv octavia-squidington-iv added the CDK Connector Development Kit label Dec 6, 2022
@maxi297
Copy link
Contributor Author

maxi297 commented Dec 6, 2022

Do we release at every pull request?

If so, I know #20019 bumps the version and should be merged soon so I'll wait for it to be merge before working on updating the version

@brianjlai
Copy link
Contributor

There isn't a strict rule and on occasion we'll package up multiple PRs into the same release for convenience, so if you want them grouped because the other PR is going out, thats fine. Just make sure the changelog reflects both changes whether you include the blurb in this PR or the other that is actually doing the release

Copy link
Contributor

@brianjlai brianjlai left a comment

Choose a reason for hiding this comment

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

changes look good! nice job cleaning up this method too, I don't quite remember why we had originally wrote this method in a confusing way with the multiple cursor field comparisons against the start date

@maxi297
Copy link
Contributor Author

maxi297 commented Dec 8, 2022

/publish-cdk dry-run=true

🕑 https://github.com/airbytehq/airbyte/actions/runs/3649009000
https://github.com/airbytehq/airbyte/actions/runs/3649009000

@maxi297
Copy link
Contributor Author

maxi297 commented Dec 8, 2022

/publish-cdk dry-run=false

🕑 https://github.com/airbytehq/airbyte/actions/runs/3649066583
https://github.com/airbytehq/airbyte/actions/runs/3649066583

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CDK Connector Development Kit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants