-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
connector builder: Set state on stream slices #37109
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
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.
looks good to me. I find it curious that we've had state
in StreamReadSlices
and our open API spec this whole time, but we just never bothered to populate it until now
elif message.type == MessageType.STATE: | ||
latest_state_message = message.state |
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.
@girarda my understanding of this feature was that every slice would output its own state message, so we would expect every item in the slices
response of the read_stream
request to contain a state
object.
But when I tested this, only the last slice had a state
in the response of read_stream
.
Is this expected that only the last slice will ever output a state message? If so, why does this happen instead of each slice outputting a state?
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.
how did you test this? My initial hypothesis is this might be because we don't necessarily produce a stream slice due to record and page limits but I can dig more into the issue
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.
I used the exchange rates connector that you build in the tutorial:
exchange_rates.yaml
And I pointed my local connector builder server at the local CDK on this branch
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 issue is that the builder server submits a request for a full refresh read instead of incremental. Fix is here https://github.com/airbytehq/airbyte-platform-internal/pull/12179
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.
With the fix for the sync mode in https://github.com/airbytehq/airbyte-platform-internal/pull/12179, this is now working as expected for me when used with the Builder
What
Update the builder's message grouper to set the state on the output stream slices.
The BE server does not need to be updated, but we'll need to update the webapp to display them.
Solves a part of https://github.com/airbytehq/airbyte-internal-issues/issues/1561
How
Example response from the server:
![Screenshot 2024-04-15 at 10 24 53 AM](https://private-user-images.githubusercontent.com/1451943/322572258-e30fab35-8685-4995-821e-1b91aef11c62.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjA0NjI0NjEsIm5iZiI6MTcyMDQ2MjE2MSwicGF0aCI6Ii8xNDUxOTQzLzMyMjU3MjI1OC1lMzBmYWIzNS04Njg1LTQ5OTUtODIxZS0xYjkxYWVmMTFjNjIucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI0MDcwOCUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNDA3MDhUMTgwOTIxWiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9Yjk2YjVkZDU0NzcwYzQxZGU0YjdhMDZhMjE3YWRlMzc1MWZhZWQwZTBiODI2ZWM3NTMwZjYwYmIxMjAxNTFkNiZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QmYWN0b3JfaWQ9MCZrZXlfaWQ9MCZyZXBvX2lkPTAifQ.0Lm4aG5sw0JJZUSFIlk_doF-yUeEyCjG3-p02ctx2uM)