Skip to content
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

Update API in order to support the per stream state #13468

Closed
wants to merge 7 commits into from

Conversation

benmoriceau
Copy link
Contributor

What

Update the API in order to handle the per stream state (and per stream reset).

@benmoriceau benmoriceau requested a review from timroes June 3, 2022 22:57
@github-actions github-actions bot added area/api Related to the api area/documentation Improvements or additions to documentation area/platform issues related to the platform area/server labels Jun 3, 2022
Copy link
Contributor

@cgardens cgardens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

started re-reviewing this. realized that there are still a few things in the spec that need ironing out. lmk if you'd like to go over any of them!

type: array
items:
$ref: "#/components/schemas/StreamKey"
StreamKey:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reminder: replace thjs with stream descriptor

@@ -3987,8 +4023,100 @@ components:
$ref: "#/components/schemas/ConnectionId"
state:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

switch this to legacy?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am wondering about compatibility issues. I have no idea about how this endpoint is used (apart from it not being use in the UI). I some users have a script fetching the state, it should remains the same.

airbyte-api/src/main/openapi/config.yaml Show resolved Hide resolved
type: string
enum:
- global
- per_stream
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- per_stream
- stream

@@ -3987,8 +4023,100 @@ components:
$ref: "#/components/schemas/ConnectionId"
state:
$ref: "#/components/schemas/ConnectionStateObject"
globalState:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is out of date.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@benmoriceau benmoriceau temporarily deployed to more-secrets June 9, 2022 20:16 Inactive
@benmoriceau benmoriceau temporarily deployed to more-secrets June 9, 2022 21:02 Inactive
@benmoriceau benmoriceau marked this pull request as ready for review June 10, 2022 17:39
@benmoriceau benmoriceau temporarily deployed to more-secrets June 10, 2022 17:41 Inactive
@benmoriceau benmoriceau temporarily deployed to more-secrets June 10, 2022 17:52 Inactive
@benmoriceau benmoriceau temporarily deployed to more-secrets June 10, 2022 17:56 Inactive
@cgardens
Copy link
Contributor

closing in favor #13835 so that other people can review it.

@cgardens cgardens closed this Jun 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api Related to the api area/documentation Improvements or additions to documentation area/platform issues related to the platform area/server
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants