Update state message #12586

benmoriceau · 2022-05-04T20:45:38Z

What

Update the airbyte state message to support the per stream state.

The state message still contains the old way of storing the state in the data fields. It introduce 2 new fields to represent the global and the per stream state.

The new state message has been defined in: https://docs.google.com/document/d/1zJVUQkVy8ZJUcqzM_hYPGijr4pI0Xx6I2eqpagJ2r7E/edit#heading=h.voxzl9lf9qd1

sherifnada

@benmoriceau was it discussed whether the connector should declare via spec.json if it manages global or per-stream state? this would allow transparency to the user about whether a table reset they perform in the UI will nuke their whole dataset vs. just one stream. Or is the idea that this determination would happen by reading the STATE message?

sherifnada · 2022-05-04T23:02:34Z

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

+      The type of state the other fields represent.
+      If not set, the state data is interpreted as GLOBAL and should be read from the `data` field for backwards compatibility.
+      GLOBAL means that the state should be read from `global` and means that it represents the state for all the streams.
+      PER_STREAM means that the state should be read from `streams`. Each item in the ist represents the state for the associated stream.


Suggested change

PER_STREAM means that the state should be read from `streams`. Each item in the ist represents the state for the associated stream.

PER_STREAM means that the state should be read from `streams`. Each item in the list represents the state for the associated stream.

sherifnada · 2022-05-04T23:03:03Z

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

+    description: "per stream state data"
+    additionalProperties: false
+    properties:
+      name:


Suggested change

name:

stream_name:

isn't it redundant with the name of the fields in the AirbyteSateMessage it will be something like stream[x].stream_name.

I don't feel strongly on this one, but I think I do lean towards sherif's suggestion. I don't think it's redundant because when you say an AirbyteStreamState has a name it isn't necessarily obvious that that name is a stream name. It could be interpreted as AirbyteStreamStates just have a name for some reason. doing with stream_name removes the ambiguity.

sherifnada · 2022-05-04T23:19:07Z

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

+      global:
+        "$ref": "#/definitions/AirbyteStateBlob"
+      streams:
+        type: array


why is this an array instead of a map?

The map type doesn't exist in json schema. One version was using the additional properties for it but it was not clear: https://json-schema.org/draft/2020-12/json-schema-core.html#rfc.section.4.2.1

would "additionalProperties": { "type": AirbyteStateBlob } work with object definition?

I think it is worth exploring something like what Sherif proposed above. Otherwise we will need to need to sort through all of the streamStates in this array to find specific ones, rather than having direct access to a stream state given the stream name

would "additionalProperties": { "type": AirbyteStateBlob } work with object definition?

This would work but it is not explicit that the key is a string plus the field is gonna be named additionalProperties and not streams (doc). We considered it at some point but abandoned it because of all the confusion it was bringing.
The tradeoff of rebuilding it as a map sounds better than having a field that it is not properly named and which can lead to have something unexpected in it.

Maps in JSONSchema have been pretty sus. i.e. it seems like a lot of the codegen doesn't know what to do with them. I think the array is going to lead to less headache.

Lake makes a good point on the sorting, but I think we can handle it.

lmossman · 2022-05-05T01:15:22Z

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

-    additionalProperties: true
-    required:
-      - data
+    additionalProperties: false


Why are we changing this to false? I think that will make it harder in the future to add/remove fields to this object in a backwards-compatible way

This field allows too many thing it is a Map<String, Object>, we can't know what is in it and we should avoid using it. I don't think it is being used at the moment and we shouldn't be using it in the future.

lmossman · 2022-05-05T01:18:35Z

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

+      global:
+        "$ref": "#/definitions/AirbyteStateBlob"
+      streams:
+        type: array


I think it is worth exploring something like what Sherif proposed above. Otherwise we will need to need to sort through all of the streamStates in this array to find specific ones, rather than having direct access to a stream state given the stream name

lmossman · 2022-05-05T01:21:00Z

@benmoriceau was it discussed whether the connector should declare via spec.json if it manages global or per-stream state? this would allow transparency to the user about whether a table reset they perform in the UI will nuke their whole dataset vs. just one stream. Or is the idea that this determination would happen by reading the STATE message?

I will let Benoit confirm, but AFAIK the idea was that this would be determined by reading the most recent state from the connector, defaulting to GLOBAL if there is no state for that connector. Once we process the first PER_STREAM state message from that connector, it would change the behavior in the UI.

CLAassistant · 2022-05-05T13:07:20Z

All committers have signed the CLA.

benmoriceau · 2022-05-05T16:55:54Z

I will let Benoit confirm, but AFAIK the idea was that this would be determined by reading the most recent state from the connector, defaulting to GLOBAL if there is no state for that connector. Once we process the first PER_STREAM state message from that connector, it would change the behavior in the UI.

We spoke about that offline with Sherif. Some connector will support both global and per stream update. The way we will know which state to pass will be by using the state table and the TBD per stream state table and see which one is the active one. As you say the default state will be GLOBAL if no state is present.

…-state-message

cgardens

lgtm. i think it is worth doing name => stream_name

cgardens · 2022-05-11T22:57:09Z

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

    properties:
+      state_type:


we can't make this required because it wouldn't be backwards compatible, right?

cgardens · 2022-05-11T23:01:59Z

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

+      global:
+        "$ref": "#/definitions/AirbyteStateBlob"
+      streams:
+        type: array


Maps in JSONSchema have been pretty sus. i.e. it seems like a lot of the codegen doesn't know what to do with them. I think the array is going to lead to less headache.

Lake makes a good point on the sorting, but I think we can handle it.

cgardens · 2022-05-11T23:02:59Z

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

+    description: "per stream state data"
+    additionalProperties: false
+    properties:
+      name:


I don't feel strongly on this one, but I think I do lean towards sherif's suggestion. I don't think it's redundant because when you say an AirbyteStreamState has a name it isn't necessarily obvious that that name is a stream name. It could be interpreted as AirbyteStreamStates just have a name for some reason. doing with stream_name removes the ambiguity.

What Update the airbyte state message to support the per stream state. The state message still contains the old way of storing the state in the data fields. It introduce 2 new fields to represent the global and the per stream state.

Update state message

46355b4

benmoriceau requested review from lmossman and michel-tricot May 4, 2022 20:45

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

benmoriceau requested review from sherifnada and grishick May 4, 2022 22:44

sherifnada reviewed May 4, 2022

View reviewed changes

lmossman reviewed May 5, 2022

View reviewed changes

Fix typo

55ce25a

benmoriceau temporarily deployed to more-secrets May 5, 2022 17:08 Inactive

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

9f71b91

…-state-message

benmoriceau requested review from lmossman and sherifnada May 5, 2022 17:30

benmoriceau temporarily deployed to more-secrets May 5, 2022 17:34 Inactive

benmoriceau temporarily deployed to more-secrets May 5, 2022 17:35 Inactive

benmoriceau temporarily deployed to more-secrets May 5, 2022 18:44 Inactive

benmoriceau temporarily deployed to more-secrets May 5, 2022 19:40 Inactive

benmoriceau added 2 commits May 9, 2022 10:05

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

5856b39

…-state-message

Restore additional properties

4db4af4

benmoriceau temporarily deployed to more-secrets May 9, 2022 17:08 Inactive

State message update.

748ed13

benmoriceau temporarily deployed to more-secrets May 10, 2022 15:58 Inactive

benmoriceau added 2 commits May 10, 2022 13:27

Add deprecated tag to a deprecated field.

870034c

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

2d34be7

…-state-message

benmoriceau temporarily deployed to more-secrets May 10, 2022 20:31 Inactive

benmoriceau requested a review from cgardens May 11, 2022 20:12

cgardens approved these changes May 11, 2022

View reviewed changes

benmoriceau merged commit b3373b9 into master May 12, 2022

benmoriceau deleted the bmoric/new-state-message branch May 12, 2022 16:00

pedroslopez mentioned this pull request May 12, 2022

update CDK airbyte protocol models to fix master build #12829

Merged

octavia-squidington-iii mentioned this pull request May 13, 2022

Bump Airbyte version from 0.38.2-alpha to 0.38.3-alpha #12839

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update state message #12586

Update state message #12586

benmoriceau commented May 4, 2022 •

edited

Loading

sherifnada left a comment

sherifnada May 4, 2022

benmoriceau May 5, 2022

sherifnada May 4, 2022

benmoriceau May 5, 2022 •

edited

Loading

cgardens May 11, 2022

sherifnada May 4, 2022

benmoriceau May 4, 2022

sherifnada May 4, 2022

lmossman May 5, 2022

benmoriceau May 5, 2022 •

edited

Loading

cgardens May 11, 2022

lmossman May 5, 2022

benmoriceau May 5, 2022

lmossman May 5, 2022

lmossman commented May 5, 2022

CLAassistant commented May 5, 2022 •

edited

Loading

benmoriceau commented May 5, 2022

cgardens left a comment

cgardens May 11, 2022

cgardens May 11, 2022

cgardens May 11, 2022

	PER_STREAM means that the state should be read from `streams`. Each item in the ist represents the state for the associated stream.
	PER_STREAM means that the state should be read from `streams`. Each item in the list represents the state for the associated stream.

Update state message #12586

Update state message #12586

Conversation

benmoriceau commented May 4, 2022 • edited Loading

What

sherifnada left a 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

benmoriceau May 5, 2022 • edited Loading

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

benmoriceau May 5, 2022 • edited Loading

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

lmossman commented May 5, 2022

CLAassistant commented May 5, 2022 • edited Loading

benmoriceau commented May 5, 2022

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

Choose a reason for hiding this comment

benmoriceau commented May 4, 2022 •

edited

Loading

benmoriceau May 5, 2022 •

edited

Loading

benmoriceau May 5, 2022 •

edited

Loading

CLAassistant commented May 5, 2022 •

edited

Loading