Update schema #13573

benmoriceau · 2022-06-07T21:03:44Z

What

New modification of the schema.

…te-message-model-change

cgardens · 2022-06-09T19:04:25Z

airbyte-cdk/python/airbyte_cdk/models/airbyte_protocol.py

@@ -1,7 +1,3 @@
-#


the changes in this file look sus. formatting / linting didn't run?

Yes, I manually run the generate script and it created this changes. I have posted on the connector channel about it.

they responded. sounds like you just need to run build.

yeah, I managed to finished on build. it is fixed

cgardens · 2022-06-09T19:05:03Z

airbyte-protocol/protocol-models/src/main/resources/airbyte_protocol/airbyte_protocol.yaml

+      GLOBAL means that the state should be read from `global` and means that it represents the state for all the streams. It contains one shared
+      state and individual stream states.
+      PER_STREAM means that the state should be read from `stream`. The state present in this field correspond to the isolated state of the
+      associated stream description.
    enum:
      - GLOBAL
      - PER_STREAM


Suggested change

- PER_STREAM

- STREAM

- LEGACY

per @jdpgrailsdev comment let's add legacy

also i thought we were just calling it STREAM, not PER_STREAM

also good to mention in the description that is the same as LEGACY.

airbyte-protocol/protocol-models/src/main/resources/airbyte_protocol/airbyte_protocol.yaml

cgardens · 2022-06-09T19:06:57Z

airbyte-protocol/protocol-models/src/main/resources/airbyte_protocol/airbyte_protocol.yaml

+          "$ref": "#/definitions/AirbyteStreamState"
+  StreamDescriptor:
+    type: object
+    additionalProperties: false


is there any reason not to set this to true? i dunno if we'll ever change how we identify a stream, but i'm not sure there's any value to locking ourselves in.

I wanted to avoid having the object to generate the extra field additionalProperties where it is not needed. I set it to true for future compatibility.

cgardens · 2022-06-09T19:07:39Z

airbyte-protocol/protocol-models/src/main/resources/airbyte_protocol/airbyte_protocol.yaml

      state:
        "$ref": "#/definitions/AirbyteStateBlob"
+  AirbyteGlobalState:
+    type: object
+    additionalProperties: false


any reason not to set it to true? I thought generally in the protocol we set this to true for compatibility reasons.

cgardens · 2022-06-09T19:09:36Z

airbyte-protocol/protocol-models/src/main/resources/airbyte_protocol/airbyte_protocol.yaml

+    required:
+      - sharedState
+    properties:
+      sharedState:


cgardens · 2022-06-09T19:09:38Z

airbyte-protocol/protocol-models/src/main/resources/airbyte_protocol/airbyte_protocol.yaml

+    type: object
+    additionalProperties: false
+    required:
+      - sharedState


also i don't think this should be required. i think it is stream_states that should be required.

cgardens · 2022-06-09T19:09:44Z

airbyte-protocol/protocol-models/src/main/resources/airbyte_protocol/airbyte_protocol.yaml

+    properties:
+      sharedState:
+        "$ref": "#/definitions/AirbyteStateBlob"
+      streamStates:


airbyte-protocol/protocol-models/src/main/resources/airbyte_protocol/airbyte_protocol.yaml

cgardens · 2022-06-09T19:11:14Z

airbyte-protocol/protocol-models/src/main/resources/airbyte_protocol/airbyte_protocol.yaml

-        description: "Stream name"
-        type: string
+      streamDescriptor:
+        "$ref": "#/definitions/StreamDescriptor"
      state:


in the spec we called it stream_state. any reason not to stick with that name?

It felt like duplication ($.stream.stream_state vs $.stream.state). But I have renamed it because it describe what state is being stored in a better way.

…te-message-model-change

airbyte-protocol/protocol-models/src/main/resources/airbyte_protocol/airbyte_protocol.yaml

cgardens

couple last comments!

cgardens · 2022-06-10T00:19:57Z

airbyte-protocol/protocol-models/src/main/resources/airbyte_protocol/airbyte_protocol.yaml

+    additionalProperties: true
+    required:
+      - stream_descriptor
+      - stream_state


as written this will fail validations if stream_set: null. I think we want to be able to do that. so i think it can't be required.

I guess you are speaking of stream_state. On the DB, if the state is null we will remove it from the DB, so it won't be present in the message, that's why it is required. Is is some scenario that was not expected.

yes. sorry. stream_state.

when a connectors wants to reset the state of a stream, I expected it would look like:

type: stream stream: stream_descriptor: name: accounts stream_state: null

how were you thinking about it?

I re-request a review even if there is no change at the moment.

cgardens

looks great! thanks, @benmoriceau

airbyte-protocol/protocol-models/src/main/resources/airbyte_protocol/airbyte_protocol.yaml

cgardens · 2022-06-10T23:38:37Z

wahoo!!!

Update schema

bb99d0a

github-actions bot added area/platform issues related to the platform area/protocol labels Jun 7, 2022

benmoriceau temporarily deployed to more-secrets June 7, 2022 21:05 Inactive

Merge branch 'master' of github.com:airbytehq/airbyte into bmoric/sta…

87c140d

…te-message-model-change

benmoriceau temporarily deployed to more-secrets June 9, 2022 16:25 Inactive

generate python

a7fdf2c

github-actions bot added the CDK Connector Development Kit label Jun 9, 2022

benmoriceau temporarily deployed to more-secrets June 9, 2022 16:43 Inactive

benmoriceau marked this pull request as ready for review June 9, 2022 17:13

benmoriceau requested review from girarda, pedroslopez, cgardens and jdpgrailsdev and removed request for jdpgrailsdev and cgardens June 9, 2022 17:29

Stream as an object

f518165

benmoriceau temporarily deployed to more-secrets June 9, 2022 17:37 Inactive

benmoriceau requested review from cgardens and jdpgrailsdev June 9, 2022 17:37

cgardens requested changes Jun 9, 2022

View reviewed changes

benmoriceau added 2 commits June 9, 2022 16:51

Merge branch 'master' of github.com:airbytehq/airbyte into bmoric/sta…

2e4e4de

…te-message-model-change

PR comments

535c3e2

benmoriceau temporarily deployed to more-secrets June 10, 2022 00:08 Inactive

cgardens reviewed Jun 10, 2022

View reviewed changes

airbyte-protocol/protocol-models/src/main/resources/airbyte_protocol/airbyte_protocol.yaml Show resolved Hide resolved

generate python

311eeb9

cgardens requested changes Jun 10, 2022

View reviewed changes

benmoriceau temporarily deployed to more-secrets June 10, 2022 01:05 Inactive

benmoriceau requested a review from cgardens June 10, 2022 01:10

rm unused required

9a48600

benmoriceau temporarily deployed to more-secrets June 10, 2022 18:39 Inactive

cgardens approved these changes Jun 10, 2022

View reviewed changes

airbyte-protocol/protocol-models/src/main/resources/airbyte_protocol/airbyte_protocol.yaml Show resolved Hide resolved

Description the state with no type

4619cd7

benmoriceau temporarily deployed to more-secrets June 10, 2022 20:49 Inactive

benmoriceau force-pushed the bmoric/state-message-model-change branch from 9e27a43 to 4619cd7 Compare June 10, 2022 20:49

benmoriceau temporarily deployed to more-secrets June 10, 2022 20:52 Inactive

cgardens approved these changes Jun 10, 2022

View reviewed changes

Fix connector build

dfa572a

benmoriceau temporarily deployed to more-secrets June 10, 2022 21:34 Inactive

Format

46e5c95

benmoriceau temporarily deployed to more-secrets June 10, 2022 21:36 Inactive

format

4a7f256

cgardens temporarily deployed to more-secrets June 10, 2022 23:11 Inactive

benmoriceau merged commit 704dd8b into master Jun 10, 2022

benmoriceau deleted the bmoric/state-message-model-change branch June 10, 2022 23:36

octavia-squidington-iii mentioned this pull request Jun 14, 2022

Bump Airbyte version from 0.39.17-alpha to 0.39.18-alpha #13759

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update schema #13573

Update schema #13573

benmoriceau commented Jun 7, 2022

cgardens Jun 9, 2022

benmoriceau Jun 9, 2022

cgardens Jun 10, 2022

benmoriceau Jun 10, 2022

cgardens Jun 9, 2022

cgardens Jun 9, 2022

cgardens Jun 9, 2022

cgardens Jun 9, 2022

benmoriceau Jun 9, 2022

cgardens Jun 9, 2022

benmoriceau Jun 9, 2022

cgardens Jun 9, 2022

cgardens Jun 9, 2022

benmoriceau Jun 9, 2022

cgardens Jun 9, 2022

cgardens Jun 9, 2022

benmoriceau Jun 9, 2022

cgardens Jun 9, 2022

benmoriceau Jun 9, 2022

cgardens Jun 9, 2022

benmoriceau Jun 10, 2022

cgardens left a comment

cgardens Jun 10, 2022

benmoriceau Jun 10, 2022

cgardens Jun 10, 2022 •

edited

Loading

benmoriceau Jun 10, 2022

cgardens left a comment

cgardens commented Jun 10, 2022

Update schema #13573

Update schema #13573

Conversation

benmoriceau commented Jun 7, 2022

What

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgardens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgardens Jun 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgardens left a comment

Choose a reason for hiding this comment

cgardens commented Jun 10, 2022

cgardens Jun 10, 2022 •

edited

Loading