🐛 Source Chargebee: Ensure no pagination issues during concurrency #48510
+344
−284
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
octavia-squidington-iii
added
area/connectors
octavia-squidington-iii
added
the
area/documentation
maxi297
commented
Nov 15, 2024
| | 0.7.1 | 2024-11-04 | [48133](https://github.com/airbytehq/airbyte/pull/48133) | Fix `error message pattern` to handle `Product 1.0` related errors | | ||
| | 0.7.0 | 2024-10-30 | [47978](https://github.com/airbytehq/airbyte/pull/47978) | Upgrade the CDK and startup files to sync incremental streams concurrently | | ||
| | 0.6.18 | 2024-10-31 | [47099](https://github.com/airbytehq/airbyte/pull/47099) | Update dependencies | | ||
| | 0.7.2 | 2024-11-18 | [48510](https://github.com/airbytehq/airbyte/pull/48510) | Ensure no pagination issues on concurrent syncs | |
There was a problem hiding this comment.
I set up the date for Monday but if we think it is necessary, I can release today
maxi297
commented
Nov 15, 2024
| @@ -35,8 +35,8 @@ definitions: | |||
| datetime: "{{ format_datetime(config['start_date'], '%s') }}" | |||
| datetime_format: "%s" | |||
| end_datetime: | |||
| datetime: "{{ now_utc().strftime('%s') }}" | |||
| datetime_format: "%s" | |||
| datetime: "{{ now_utc().strftime('%Y-%m-%dT%H:%M:%SZ') }}" | |||
There was a problem hiding this comment.
I got trolled by that. As this is not expose anywhere, I've decided to update the format to one that won't break with timezone differences.
| @@ -195,7 +195,7 @@ def test_given_no_initial_state_when_read_then_return_state_based_on_most_recent | |||
| output = self._read(_config().with_start_date(self._start_date - timedelta(hours=8)), _NO_STATE) | |||
| most_recent_state = output.most_recent_state | |||
| assert most_recent_state.stream_descriptor == StreamDescriptor(name=_STREAM_NAME) | |||
| assert most_recent_state.stream_state == AirbyteStateBlob(updated_at=cursor_value) | |||
| assert most_recent_state.stream_state == AirbyteStateBlob(updated_at=str(cursor_value)) | |||
There was a problem hiding this comment.
The state has changed from a int to a string. This shouldn't be an issue because as shown here, the change is backward compatible
brianjlai
reviewed
Nov 16, 2024
There was a problem hiding this comment.
code looks straightforward and nice catch on the timezone thing on the manifest.
But seeing some acceptance test failures and regression tests as well. I think the abnormal state test we need to update the future_state.json file to use strings instead of int. But not sure about the others. One looks like a timeout.
Have you run other regression tests that passed on chargebee w/ the latest commit?
|
CATs seem to have been more than flaky recently (see this). I would assume the rate limiting does not help. What worries me a bit is that our sandbox account right now is failing although it seems like a destination issue... I've executed regression testing locally with our sandbox account and got pretty good results: However, I had This is strange because I definitely can see those stream statuses in the output: From looking deeper at this test, it seems like for each record, we validate that all streams in the catalog have emitted a stream status. There are two things which are surprising to me:
|
@maxi297 |
|
@alafanechere I'll therefore comment the lines that are validating this because it prevents the other validations from test_read to occur. Let me re-run this |
|
Without this check, the I've reduced the num_workers in our GSM config and will retrigger the tests |
maxi297
commented
Nov 20, 2024
| $parameters: | ||
| name: "subscription_with_scheduled_changes" | ||
| primary_key: "id" | ||
| primary_key: "subscription_id" |
There was a problem hiding this comment.
This is normally a breaking change but since it wasn't working before (id wasn't populated), then I won't make it a breaking change
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What
As part of this release, there is a bugfix regarding pagination and threads safety. We want to make sure source-chargebee is not affected by it and therefore we will re-release it with the CDK version.
How
Update dependencies
User Impact
If users were facing pagination issues, this should fix it.
Can this PR be safely reverted and rolled back?