🐛 Source Stripe: do not emit empty state messages #30660

davydov-d · 2023-09-21T13:33:26Z

What

Emit non-empty state messages
https://github.com/airbytehq/oncall/issues/3004

How

Bugfix: do not return empty state when that same empty state was passed in. Instead, return the cursor value from the latest record
Another bugfix for streams that did not support incremental syncs until version 4.0.0 and do not have a cursor value in a full refresh or initial incremental sync mode: fill the cursor value with current timestamp
Added unit tests
Minor doc update
Updated stream schema + expected records for the build to pass

🚨 User Impact 🚨

No impact

vercel · 2023-09-21T13:33:32Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment

Name	Status	Preview	Comments	Updated (UTC)
airbyte-docs	⬜️ Ignored (Inspect)	Visit Preview		Sep 21, 2023 7:06pm

github-actions · 2023-09-21T13:33:50Z

Before Merging a Connector Pull Request

Wow! What a great pull request you have here! 🎉

To merge this PR, ensure the following has been done/considered for each connector added or updated:

PR name follows PR naming conventions
Breaking changes are considered. If a Breaking Change is being introduced, ensure an Airbyte engineer has created a Breaking Change Plan.
Connector version has been incremented in the Dockerfile and metadata.yaml according to our Semantic Versioning for Connectors guidelines
You've updated the connector's metadata.yaml file any other relevant changes, including a breakingChanges entry for major version bumps. See metadata.yaml docs
Secrets in the connector's spec are annotated with airbyte_secret
All documentation files are up to date. (README.md, bootstrap.md, docs.md, etc...)
Changelog updated in docs/integrations/<source or destination>/<name>.md with an entry for the new version. See changelog example
Migration guide updated in docs/integrations/<source or destination>/<name>-migrations.md with an entry for the new version, if the version is a breaking change. See migration guide example
If set, you've ensured the icon is present in the platform-internal repo. (Docs)

If the checklist is complete, but the CI check is failing,

Check for hidden checklists in your PR description
Toggle the github label checklist-action-run on/off to re-run the checklist CI.

airbyte-oss-build-runner · 2023-09-21T13:53:31Z

source-stripe test report (commit `58e4fe4606`) - ❌

⏲️ Total pipeline duration: 11mn57s

Step	Result
Connector package install	✅
Build source-stripe docker image for platform linux/x86_64	✅
Unit tests	✅
Acceptance tests	❌
Code format checks	✅
Validate airbyte-integrations/connectors/source-stripe/metadata.yaml	✅
Connector version semver check	✅
Connector version increment check	✅
QA checks	✅

🔗 View the logs here

☁️ View runs for commit in Dagger Cloud

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=source-stripe test

airbyte-oss-build-runner · 2023-09-21T15:06:05Z

source-stripe test report (commit `c65e13fdfa`) - ✅

⏲️ Total pipeline duration: 11mn57s

Step	Result
Connector package install	✅
Build source-stripe docker image for platform linux/x86_64	✅
Unit tests	✅
Acceptance tests	✅
Code format checks	✅
Validate airbyte-integrations/connectors/source-stripe/metadata.yaml	✅
Connector version semver check	✅
Connector version increment check	✅
QA checks	✅

🔗 View the logs here

☁️ View runs for commit in Dagger Cloud

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=source-stripe test

lazebnyi

LGTM

airbyte-integrations/connectors/source-stripe/source_stripe/streams.py

…reams.py Co-authored-by: Serhii Lazebnyi <53845333+lazebnyi@users.noreply.github.com>

airbyte-oss-build-runner · 2023-09-21T16:46:52Z

source-stripe test report (commit `cb9039b07a`) - ❌

⏲️ Total pipeline duration: 11mn52s

Step	Result
Connector package install	✅
Build source-stripe docker image for platform linux/x86_64	✅
Unit tests	✅
Acceptance tests	✅
Code format checks	❌
Validate airbyte-integrations/connectors/source-stripe/metadata.yaml	✅
Connector version semver check	✅
Connector version increment check	✅
QA checks	✅

🔗 View the logs here

☁️ View runs for commit in Dagger Cloud

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=source-stripe test

airbyte-oss-build-runner · 2023-09-21T19:20:11Z

source-stripe test report (commit `57c3dcd5f3`) - ✅

⏲️ Total pipeline duration: 12mn08s

Step	Result
Connector package install	✅
Build source-stripe docker image for platform linux/x86_64	✅
Unit tests	✅
Acceptance tests	✅
Code format checks	✅
Validate airbyte-integrations/connectors/source-stripe/metadata.yaml	✅
Connector version semver check	✅
Connector version increment check	✅
QA checks	✅

🔗 View the logs here

☁️ View runs for commit in Dagger Cloud

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=source-stripe test

maxi297

There is a couple of things I don't understand enough to give approval. With the answer to my question, I should be able to approve

maxi297 · 2023-09-22T13:29:49Z

airbyte-integrations/connectors/source-stripe/source_stripe/streams.py

+ current_cursor_value = record.get(self.legacy_cursor_field, pendulum.now().int_timestamp)
+
+ # yield the record with the added cursor_field
+ yield record | {self.cursor_field: current_cursor_value}


Defaulting on now seems odd to me. Is it only some records in a stream that might not have self.cursor_field and self.legacy_cursor_field in the record? I would be afraid of the following series of events:

Consume a record #⁠1 self.cursor_field and self.legacy_cursor_field not in the record so we use now

checkpointing from AbstractSource

Consume a record #⁠2 with self.cursor_field or self.legacy_cursor_field less than now

Sync crash

In that case, we would start the next sync at the now when we consumed record #⁠1 but would potentially miss record #⁠2 and others. The only reason allow for now as the default is if all the records for the same stream will not have self.legacy_cursor_field as a field. Can you confirm?

@maxi297 yes, the case here is some streams that do not have a cursor value at all when running in a full refresh mode (or initial incremental sync), so the consequences you described are not relevant

maxi297 · 2023-09-22T13:30:22Z

airbyte-integrations/connectors/source-stripe/source_stripe/streams.py

- yield record
+ if self.cursor_field in record:
+ yield record
+ continue # Skip the rest of the loop iteration


nit: Would it be more readable to use the else statement instead of having to add a comment?

matter of taste :) I prefer early exit/continue over else statements

but if you insist, I can change it of course

There are a couple of resources regarding cognitive complexity recommending to avoid "jumps to" labels. One example is Sonar I personally agree with these but I'll not enforce it if the maintainers of the code prefer otherwise

Thank you! I will leave it here as is, but take into account in the future updates

airbyte-integrations/connectors/source-stripe/source_stripe/streams.py

airbyte-integrations/connectors/source-stripe/unit_tests/test_streams.py

davydov-d · 2023-09-22T15:56:46Z

@maxi297 can you please take a look at the comments

maxi297

Given the added information by @davydov-d, I have enough information to approve this change

OC #3004 Source Stripe: fix incremental sync state

8fc09e7

octavia-squidington-iii added area/connectors Connector related issues area/documentation Improvements or additions to documentation connectors/source/stripe labels Sep 21, 2023

update changelog

58e4fe4

davydov-d changed the title ~~OC #3004 Source Stripe: fix incremental sync state~~ 🐛 Source Stripe: do not emit empty state messages Sep 21, 2023

davydov-d marked this pull request as ready for review September 21, 2023 13:39

davydov-d added the checklist-action-run label Sep 21, 2023

vercel bot deployed to Preview September 21, 2023 13:40 View deployment

davydov-d requested review from artem1205, lazebnyi and maxi297 September 21, 2023 13:40

update stream schema + expected records

c65e13f

vercel bot deployed to Preview September 21, 2023 14:53 View deployment

lazebnyi approved these changes Sep 21, 2023

View reviewed changes

airbyte-integrations/connectors/source-stripe/source_stripe/streams.py Outdated Show resolved Hide resolved

Update airbyte-integrations/connectors/source-stripe/source_stripe/st…

cb9039b

…reams.py Co-authored-by: Serhii Lazebnyi <53845333+lazebnyi@users.noreply.github.com>

fix formatting

57c3dcd

maxi297 reviewed Sep 22, 2023

View reviewed changes

davydov-d requested a review from maxi297 September 22, 2023 14:01

maxi297 approved these changes Sep 22, 2023

View reviewed changes

davydov-d merged commit 29addb8 into master Sep 23, 2023

davydov-d deleted the ddavydov/oc-3004-source-stripe-fix-incremental-sync-state branch September 23, 2023 05:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

🐛 Source Stripe: do not emit empty state messages #30660

🐛 Source Stripe: do not emit empty state messages #30660

Uh oh!

davydov-d commented Sep 21, 2023 •

edited

Loading

vercel bot commented Sep 21, 2023 •

edited

Loading

github-actions bot commented Sep 21, 2023 •

edited by davydov-d

Loading

airbyte-oss-build-runner commented Sep 21, 2023

airbyte-oss-build-runner commented Sep 21, 2023

lazebnyi left a comment

Uh oh!

airbyte-oss-build-runner commented Sep 21, 2023

airbyte-oss-build-runner commented Sep 21, 2023

maxi297 left a comment

maxi297 Sep 22, 2023

davydov-d Sep 22, 2023

maxi297 Sep 22, 2023

davydov-d Sep 22, 2023

davydov-d Sep 22, 2023

maxi297 Sep 22, 2023

davydov-d Sep 23, 2023

Uh oh!

Uh oh!

davydov-d commented Sep 22, 2023 •

edited

Loading

maxi297 left a comment

Labels

6 participants

🐛 Source Stripe: do not emit empty state messages #30660

🐛 Source Stripe: do not emit empty state messages #30660

Uh oh!

Conversation

davydov-d commented Sep 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

How

🚨 User Impact 🚨

vercel bot commented Sep 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

github-actions bot commented Sep 21, 2023 • edited by davydov-d Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Before Merging a Connector Pull Request

airbyte-oss-build-runner commented Sep 21, 2023

source-stripe test report (commit 58e4fe4606) - ❌

airbyte-oss-build-runner commented Sep 21, 2023

source-stripe test report (commit c65e13fdfa) - ✅

lazebnyi left a comment

Choose a reason for hiding this comment

Uh oh!

airbyte-oss-build-runner commented Sep 21, 2023

source-stripe test report (commit cb9039b07a) - ❌

airbyte-oss-build-runner commented Sep 21, 2023

source-stripe test report (commit 57c3dcd5f3) - ✅

maxi297 left a comment

Choose a reason for hiding this comment

maxi297 Sep 22, 2023

Choose a reason for hiding this comment

davydov-d Sep 22, 2023

Choose a reason for hiding this comment

maxi297 Sep 22, 2023

Choose a reason for hiding this comment

davydov-d Sep 22, 2023

Choose a reason for hiding this comment

davydov-d Sep 22, 2023

Choose a reason for hiding this comment

maxi297 Sep 22, 2023

Choose a reason for hiding this comment

davydov-d Sep 23, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

davydov-d commented Sep 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

maxi297 left a comment

Choose a reason for hiding this comment

Labels

6 participants

davydov-d commented Sep 21, 2023 •

edited

Loading

vercel bot commented Sep 21, 2023 •

edited

Loading

github-actions bot commented Sep 21, 2023 •

edited by davydov-d

Loading

source-stripe test report (commit `58e4fe4606`) - ❌

source-stripe test report (commit `c65e13fdfa`) - ✅

source-stripe test report (commit `cb9039b07a`) - ❌

source-stripe test report (commit `57c3dcd5f3`) - ✅

davydov-d commented Sep 22, 2023 •

edited

Loading