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

Bug/duplicate deleted lead ids #26

Merged

Conversation

m-feeser
Copy link
Contributor

Are you a current Fivetran customer?

Matt Feeser, Data Engineer, Greenhouse

What change(s) does this PR introduce?

This is a bug fix which addresses a "system glitch" in marketo which allows the same lead_id to be deleted twice. The duplicate delete causes the marketo__leads.lead_id.unique test to fail.

Did you update the CHANGELOG?

  • Yes

Does this PR introduce a breaking change?

  • Yes (please provide breaking change details below.)
  • No (please provide an explanation as to how the change is non-breaking below.)
    The int_marketo__lead model is materialized as a view, so there is no physicalization of data in play. Therefore no reprocessing of previously loaded data is necessary. Moreover, no columns from the modified CTE are exposed. The result is only used in a case statement to set the is_deleted column value.

Did you update the dbt_project.yml files with the version upgrade (please leverage standard semantic versioning)? (In both your main project and integration_tests)

  • Yes

Is this PR in response to a previously created Bug or Feature Request

How did you test the PR changes?

  • Buildkite
  • Local (please provide additional testing details below)
  1. Ran the original model against data which contained duplicate lead_id values. The resulting view int_marketo__lead contained multiple records with the same lead_id.
  2. Applied proposed fix (distinct lead_id), ran the model against the same data which contained the duplicate lead_id values. The resulting view no longer contained multiple records for the same lead_id.

Select which warehouse(s) were used to test the PR

  • BigQuery
  • Redshift
  • Snowflake
  • Postgres
  • Databricks
  • Other (provide details below)

Provide an emoji that best describes your current mood

👷

Feedback

We are so excited you decided to contribute to the Fivetran community dbt package! We continue to work to improve the packages and would greatly appreciate your feedback on our existing dbt packages or what you'd like to see next.

@m-feeser
Copy link
Contributor Author

Hi @fivetran-joemarkiewicz. Do you have an ETA on when this PR will be reviewed? We're still having regular failures in our marketo package executions and this will fix it. Thanks.

@fivetran-joemarkiewicz
Copy link
Contributor

Hi @m-feeser my apologies for the delay. I'll work to complete the review of this PR in the current week.

@fivetran-joemarkiewicz fivetran-joemarkiewicz added type:bug Something is broken or incorrect priority:p3 Affects many users; can wait status:in_review Currently in review update_type:models Primary focus requires model updates labels Jan 12, 2023
Copy link
Contributor

@fivetran-joemarkiewicz fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

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

@m-feeser the changes you are proposing look great and I can see how they will fix the system glitch you are noting while not compromising the expected outcome of the model.

I have just a few requests to make small updates to the CHANGELOG. Once those are applied, we should be able to approve and kick off the merge/release process. Let me know if you have any questions 😄

CHANGELOG.md Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
Copy link
Contributor

@fivetran-joemarkiewicz fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

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

Thanks for sharing the insight that there is no documentation around the "system glitch". Although I feel Marketo should probably document the glitch 😬.

With that, your changes look appropriate and I am comfortable approving your PR. I will now kick off our internal release process. You should see your update released in the next patch version early on Monday.

@fivetran-joemarkiewicz
Copy link
Contributor

Thanks for sharing the insight that there is no documentation around the "system glitch". Although I feel Marketo should probably document the glitch 😬.

With that, your changes look appropriate and I am comfortable approving your PR. I will now kick off our internal release process. You should see your update released in the next patch version early on Monday.

My apologies, we will be off on Monday for Martin Luther King day. You can expect this release to be live early on Tuesday. Have a great weekend!

@m-feeser
Copy link
Contributor Author

Thank you @fivetran-joemarkiewicz! 🎉

@fivetran-joemarkiewicz fivetran-joemarkiewicz merged commit 5cf1a68 into fivetran:main Jan 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority:p3 Affects many users; can wait status:in_review Currently in review type:bug Something is broken or incorrect update_type:models Primary focus requires model updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants