-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
feat(source-zendesk-chat): migrate to incremental cursors #54151
feat(source-zendesk-chat): migrate to incremental cursors #54151
Conversation
Signed-off-by: Artem Inzhyyants <[email protected]>
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Signed-off-by: Artem Inzhyyants <[email protected]>
Signed-off-by: Artem Inzhyyants <[email protected]>
Signed-off-by: Artem Inzhyyants <[email protected]>
Signed-off-by: Artem Inzhyyants <[email protected]>
Signed-off-by: Artem Inzhyyants <[email protected]>
Signed-off-by: Artem Inzhyyants <[email protected]>
Signed-off-by: Artem Inzhyyants <[email protected]>
Signed-off-by: Artem Inzhyyants <[email protected]>
This reverts commit 7b9f91d.
Signed-off-by: Artem Inzhyyants <[email protected]>
Signed-off-by: Artem Inzhyyants <[email protected]>
Signed-off-by: Artem Inzhyyants <[email protected]>
Signed-off-by: Artem Inzhyyants <[email protected]>
Signed-off-by: Artem Inzhyyants <[email protected]>
…ndesk-chat-update-cursor # Conflicts: # airbyte-integrations/connectors/source-zendesk-chat/poetry.lock
Signed-off-by: Artem Inzhyyants <[email protected]>
page_size: 1000 | ||
cursor_value: '{{ response.get("end_time", {}) }}' | ||
stop_condition: "{{ last_page_size < 1_000 }}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe: last_page_size < page_size?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, but we do not pass page_size to the jinja interpolation, that's why i had to pass it as int 1_000
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have we run regression test on this one? Can I check the results?
|
||
concurrency_level: | ||
type: ConcurrencyLevel | ||
default_concurrency: 10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The limits from Zendesk Chat seem somewhat low i.e. 200 requests per minute for the team plan. I fear we might hit this limit with 10 concurrent threads. Should we set API budget at least?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 added api budget, 200/60 = 3.3
-> we can set default concurrency to 4
later if 10
is too high.
Signed-off-by: Artem Inzhyyants <[email protected]>
after checking the results I realised that we need to implement epoch_microseconds for dateparser as well, because it is used by stateconverte, PR: airbytehq/airbyte-python-cdk#388 |
Signed-off-by: Artem Inzhyyants <[email protected]>
Cool, ping me again once the change is in this PR and regression test results are ready ❤️ |
Signed-off-by: Artem Inzhyyants <[email protected]>
regression tests failed expectedly records differ on Item root['properties']['last_login'] only. records differ on Item root['properties']['last_login'] only. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pre-emptively approving assuming the full refresh regression tests passes
What
Resolve https://github.com/airbytehq/airbyte-internal-issues/issues/11530
How
Review guide
airbyte-integrations/connectors/source-zendesk-chat/source_zendesk_chat/manifest.yaml
User Impact
Can this PR be safely reverted and rolled back?