-
Notifications
You must be signed in to change notification settings - Fork 4.5k
fix(source-intercom): use state for parent stream requests and use global state #57524
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
fix(source-intercom): use state for parent stream requests and use global state #57524
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
…properly for incremental dependency
regression test results: analyzing results:
|
/approve-regression-tests see above comment about results
|
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.
@@ -661,6 +662,7 @@ definitions: | |||
cursor_datetime_formats: | |||
- "%s" | |||
is_client_side_incremental: true | |||
global_substream_cursor: true |
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.
Confirming that this is done cause there are usually so many conversations that we assume we will hit the per partition limit and therefore we're fine with having a global state.
Benefits:
- Easier to understand state
Drawback:
- Duplication that would not have occurred for people with very small syncs where we don't this the per partition limit
What
I was investigating a couple of suspicious failures as a result of when #53187 was originally released. The failing syncs couldn't be replicated locally, but my changes put our connector more in line with how it previously worked
regression tests to follow
How
I noticed that on the original release, we forgot to indicate on the
conversation_parts
stream that state should be passed when making the parent stream request to incrementally get parent records. We did this on the deprecated custom component and might be the reason for the hanging syncs.Also, upon inspecting the manifest more, we should also be using
global_substream_cursor
because the previous implementation did not require tracking per-parent state management.I also noticed that we were having a few issues w/ hanging syncs on substreams. This is because the arbitrary/custom state that intercom used to write since the custom cursor used to write doesn't fit quite how our actual concurrent cursors work. Concurrent per-partition state requires parent state be under
parent_state.conversations
. I added a state migration to copy the parent state toparent_state.conversations
which fixed the issue so we use incoming state to reduce the amount of parent records fetched.Review guide
manifest.yaml
User Impact
Should be none, but the underlying state format might look a little different
Can this PR be safely reverted and rolled back?