chore(source-amazon-seller-partner): update to cdk v7.1.0#66485
Conversation
👋 Greetings, Airbyte Team Member!Here are some helpful tips and reminders for your convenience. Helpful Resources
PR Slash CommandsAirbyte Maintainers (that's you!) can execute the following slash commands on your PR:
|
|
Note that the only reason I wasn't able to up to CDK v7 was because of rate limiting issues here |
|
Deploy preview for airbyte-docs ready! ✅ Preview Built with commit d9c5e09. |
|
| output = self._read(stream_name, config()) | ||
|
|
||
| assert output.errors[0].trace.error.failure_type == FailureType.config_error | ||
| assert list(filter(lambda error: error.trace.error.failure_type == FailureType.config_error, output.errors)) |
There was a problem hiding this comment.
Errors were not in the same order. We're still sending the same errors though so I just checked that there were a config error and hopefully this is sufficient.
| } | ||
|
|
||
|
|
||
| @freeze_time("2017-02-25T00:00:00Z") |
There was a problem hiding this comment.
I'm not sure exactly why this is a problem now but the start date would be after the end date and this would not generate any slice so the check would fail. If we think we need to investigate more, we should take more time to understand why
There was a problem hiding this comment.
My curiosity often gets the better of me so decided to go down the rabbit hole...
My suspicion here is that right now for this stream we take the maximum of either the replication_start_date or the current time minus 2 years. But for the replication_end_date, we just take that value if its defined. So if we don't use freezegun like you have it then we have a start date of now() - 2 years (aka 2023-09-24) and an end date as defined in the config as 2017-02-25. So now we're left with 0 slices and we fail. This presumably only passed when this stream was still using the legacy declarative stream and now with our new partition generation we yield 2 slices.
But this is probably an oversight on our part when we define the incremental behavior for orders.incremental_sync.end_datetime.datetime instead of doing:
"{{ format_datetime(config['replication_end_date'] if config.get('replication_end_date') else (now_utc() - duration('PT2M')).strftime('%Y-%m-%dT%H:%M:%SZ'), '%Y-%m-%dT%H:%M:%SZ') }}"
When in reality what we may want to do is use the default now() - 2 months if replication_start_date is newer than replication_end_date. Something like that?
Either way, I think this is a bit out of scope for the moment since I think even in the old behavior we sync no records if end is older start... But just posting my glorious trip down this work of art
|
/format-fix
|
Maxime Carbonneau-Leclerc (@maxi297) Patrick Nilan (@pnilan) why were those rate-limit errors caused by the upgrade to v7 (or are they also surfacing with the current version?). How is this PR addressing those errors? I only see changes to the connector's tests |
My understanding is that we were getting rate limited on v6 as well (I don't have a proof of that because the up-to-date pipeline force pushes when there is a new version and therefore I can't find the CI that ran on v6). We could see if v6 is fine by doing a dummy PR with basically one line change in source-amazon-seller-partner and see if we hit the rate limiting as well. WDYT? Anyway, the latest CI run is passing without rate limiting so I assume this might be fine now. |
Patrick Nilan (pnilan)
requested review from
Brian Lai (brianjlai),
David Gold (dbgold17) and
Anatolii Yatsuk (tolik0)
Okay I think this was my confusion. Based on the PR description, I expected to see some change here to fix the rate limiting issue. Correct me if I'm wrong, but Patrick Nilan (@pnilan) fixed the issue with CDK v7.1.0 in this PR last week |
| } | ||
|
|
||
|
|
||
| @freeze_time("2017-02-25T00:00:00Z") |
There was a problem hiding this comment.
My curiosity often gets the better of me so decided to go down the rabbit hole...
My suspicion here is that right now for this stream we take the maximum of either the replication_start_date or the current time minus 2 years. But for the replication_end_date, we just take that value if its defined. So if we don't use freezegun like you have it then we have a start date of now() - 2 years (aka 2023-09-24) and an end date as defined in the config as 2017-02-25. So now we're left with 0 slices and we fail. This presumably only passed when this stream was still using the legacy declarative stream and now with our new partition generation we yield 2 slices.
But this is probably an oversight on our part when we define the incremental behavior for orders.incremental_sync.end_datetime.datetime instead of doing:
"{{ format_datetime(config['replication_end_date'] if config.get('replication_end_date') else (now_utc() - duration('PT2M')).strftime('%Y-%m-%dT%H:%M:%SZ'), '%Y-%m-%dT%H:%M:%SZ') }}"
When in reality what we may want to do is use the default now() - 2 months if replication_start_date is newer than replication_end_date. Something like that?
Either way, I think this is a bit out of scope for the moment since I think even in the old behavior we sync no records if end is older start... But just posting my glorious trip down this work of art
Patrick Nilan (pnilan)
deleted the
pnilan/asp/bump-cdk-version-7.1.0
branch
Co-authored-by: maxime.c <maxime@airbyte.io> Co-authored-by: Octavia Squidington III <octavia-squidington-iii@users.noreply.github.com>
5 participants
What
Review guide
n/a
User Impact
Too many requestserrors will not be categorized astransient-- no more oncall pages regarding this. :)