Adding incremental to the data model #998

cgardens · 2020-11-16T23:07:14Z

What

Adds incremental as a concept to the catalog
Splits Catalog into a R/O version copy that is emitted by the discover action and a ConfiguredCatalog that contains configuration on how a sync will happen.
Maintains "backwards compatibility". As in there is a new struct with a new name but it acts the same as the one it is replacing.
This PR just gets all of the configuration to where it needs to be, it does NOT migrate any sources / destinations to incremental.
There is especially some more work for whoever tries to figure out the singer <> airbyte catalog conversion for incremental.

Checklist

- Builds locally
- Integration tests / standard tests pass for all destinations.
- Integration tests / standard tests pass for all java sources.
- Standard tests pass for some sampling of python and singer sources.
- Acceptance tests pass locally
- Ready for "final" review. Actually looking to merge this thing.
- bump version of every connector to pick up the new model. (done source-postgres and destination-csv to get acceptance tests to pass)

Misc

Had to do some refactoring of airbyte_message.yaml and jsonschema validation. will add comments below explaining what's going on.

Please no more requests for name changes in AirbyteCatalog and AirbyteStream

docs/architecture/incremental.md

jrhizor

I like it overall. Not mergable as-is so I'm just leaving a comment.

docs/architecture/incremental.md

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

docs/architecture/incremental.md

sherifnada

modulo name comments I think this LGTM. So we are expecting users to configure incremental/full-refresh at the stream level? I think this makes sense. however our current configs expect this to be configured at the connection level so we should adjust our behavior accordingly.

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

docs/architecture/incremental.md

michel-tricot · 2020-11-17T06:03:36Z

docs/architecture/incremental.md

@@ -0,0 +1,22 @@
+# Configuration: Streams & Incremental Syncs
+
+## AirbyteStream


Just an idea. It is a bit confusing to use AirbyteCatalog for something that come from the source and something that is fed back to the source.

What about we do (while reusing types):

sources produce an AirbyteCatalog which only contains a list of stream

sources consume a ConfiguredAirbyteCatalog which contains the list of ConfiguredAirbyteStreams?

This way it is clearer that one is R/O while the other one got some configuration applied to it. ConfiguredAirbyteCatalog should ideally be backward compatible with AirbyteCatalog (not sure how long that can hold) which would mean: sync everything with full refresh.

WDYT?

I think this is a bigger change. We can try it though.

i will try this tomorrow. think it's bigger than i have brain to do now.

i think this just changes the project from something that could be done last night to something that'll take a couple days. i think it's better. i had considered doing this but thought it was worth the time since this is the chokepoint for all work on incremental. i'll reallocate folks to other stuff while we massage this. will try this approach today.

implemented this.

docs/architecture/incremental.md

michel-tricot · 2020-11-17T07:08:10Z

Complexity. Will there be edge cases that could be painful to handle?

On Mon, Nov 16, 2020 at 10:47 PM Charles ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In airbyte-protocol/models/src/main/resources/airbyte_protocol/airbyte_message.yaml <#998 (comment)>: > type: object - existingJavaType: com.fasterxml.jackson.databind.JsonNode + required: + - json_schema + - supported_sync_modes + properties: + json_schema: + description: Stream schema using Json Schema specs. + type: object + existingJavaType: com.fasterxml.jackson.databind.JsonNode + supported_sync_modes: + type: array + items: + "$ref": "#/definitions/SyncMode" + default_cursor_field: what is the risk you're worried about? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#998 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAIZ2IKBMCYAOJ4H7UODQ5DSQIMAPANCNFSM4TXZ6PWA> .

-- *Michel Tricot* Co-Founder & CEO @ Daxtarity <https://daxtarity.com/> 415-307-4864 * <https://www.linkedin.com/in/micheltricot/>*

michel-tricot · 2020-11-17T07:11:10Z

Actually both work. Just a suggestion :) Recency works well with the concept of incremental but cursor is a bit more generic. No strong feelings about this one

On Mon, Nov 16, 2020 at 10:48 PM Charles ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In airbyte-protocol/models/src/main/resources/airbyte_protocol/airbyte_message.yaml <#998 (comment)>: > + items: + "$ref": "#/definitions/SyncMode" + default_cursor_field: + description: Path to the field that will be used to determine if a record is new or modified since the last sync. If not provided by the source, the end user will have to specify the comparable themselves. + type: array + items: string + configuration: + description: Contains information about how the data should be synced. this section is configured by the end-user and is never touched by the source. + required: + - sync_mode + properties: + # backwards compatibility: if not present, assume full_refresh. + sync_mode: + "$ref": "#/definitions/SyncMode" + default: full_refresh + cursor_field: can you describe why you prefer recency over cursor? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#998 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAIZ2IO3NE7BBFDFWW7BLL3SQIMCFANCNFSM4TXZ6PWA> .

-- *Michel Tricot* Co-Founder & CEO @ Daxtarity <https://daxtarity.com/> 415-307-4864 * <https://www.linkedin.com/in/micheltricot/>*

docs/architecture/incremental.md

cgardens · 2020-11-17T21:52:48Z

@michel-tricot - here's the version with catalog and configured catalog split.

working on propagating this throughout the system now.

cgardens · 2020-11-18T01:45:30Z

airbyte-config/models/src/main/java/io/airbyte/config/AirbyteProtocolConverters.java

@@ -40,15 +41,15 @@
 // todo (cgardens) - hack, remove after we've gotten rid of Schema object.
 public class AirbyteProtocolConverters {

-  public static AirbyteCatalog toCatalog(Schema schema) {


we only ever need to convert from schema into a configured catalog (never the R/O one).

cgardens · 2020-11-18T01:47:55Z

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

+---
+"$schema": http://json-schema.org/draft-07/schema#
+"$id": https://github.com/airbytehq/airbyte/blob/master/airbyte-protocol/models/src/main/resources/airbyte_protocol/airbyte_protocol.yaml
+title: AirbyteProtocol


this file needed to be renamed and restructured so that java code gen would pick up the new ConfiguredAirbyteCatalog.

The issue is that the codegen starts from the "main" object in the file and generates all of its dependencies. ConfiguredAirbyteCatalog was not a dependency of AirbyteMessage, so it was not being generated.

We need ConfiguredAirbyteCatalog in this file because it shares an enum with AirbyteCatalog and the python code gen can't handle structs with dependencies in different files.

cgardens · 2020-11-18T01:49:29Z

airbyte-json-validation/src/main/java/io/airbyte/validation/json/JsonSchemaValidator.java

+   *        of a JsonSchema file (instead of the main object in that file).
+   * @return schema object processed from across all dependency files.
+   */
+  public static JsonNode getSchema(final File schemaFile, String definitionStructName) {


Changing the structure of the airbyte_message / airbyte_protocol file means that we need to get the schema from an object that is not the main object in the jsonschema file. So needed to rejigger this internal lib to handle the case where we want to put the jsonschema of an object defined in "definitions" instead of the main file.

michel-tricot · 2020-11-18T05:36:48Z

...yte-integrations/bases/base-java/src/main/java/io/airbyte/integrations/base/Destination.java

@@ -41,6 +41,6 @@
   *         will always be called once regardless of success or failure.
   * @throws Exception - any exception.
   */
-  DestinationConsumer<AirbyteMessage> write(JsonNode config, AirbyteCatalog catalog) throws Exception;
+  DestinationConsumer<AirbyteMessage> write(JsonNode config, ConfiguredAirbyteCatalog catalog) throws Exception;


Looks so much better!

michel-tricot · 2020-11-18T05:48:36Z

Terrific!

cgardens · 2020-11-18T18:04:27Z

cool. going to start pushing new images then!

This reverts commit 29692b4.

cgardens requested review from jrhizor and sherifnada November 16, 2020 23:15

cgardens commented Nov 16, 2020

View reviewed changes

docs/architecture/incremental.md Outdated Show resolved Hide resolved

jrhizor reviewed Nov 16, 2020

View reviewed changes

cgardens commented Nov 16, 2020

View reviewed changes

docs/architecture/incremental.md Outdated Show resolved Hide resolved

sherifnada reviewed Nov 16, 2020

View reviewed changes

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

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

michel-tricot requested changes Nov 17, 2020

View reviewed changes

ChristopheDuong reviewed Nov 17, 2020

View reviewed changes

docs/architecture/incremental.md Outdated Show resolved Hide resolved

cgardens requested a review from michel-tricot November 17, 2020 21:52

cgardens changed the title ~~RFC: Adding incremental to the data model.~~ Adding incremental to the data model Nov 18, 2020

cgardens requested review from jrhizor, sherifnada and ChristopheDuong November 18, 2020 01:44

cgardens commented Nov 18, 2020

View reviewed changes

michel-tricot approved these changes Nov 18, 2020

View reviewed changes

jrhizor approved these changes Nov 18, 2020

View reviewed changes

cgardens added 7 commits November 18, 2020 11:25

wip

fa8a700

add more explanatino about a source

360bb9f

more

94c001e

yolo

6d5b180

fix array

06a22c9

remove docs

1f967fc

revert to master

dd937d8

cgardens added 21 commits November 18, 2020 11:25

add sync modes to airbyte stream

2237256

add configured catalog stream

ce6685b

fix

261fd9b

fix structure of airbyte_message to handle struct not in airbyte message

82d8aed

add configured catalog to standard sync input

c9b7e53

handle schemas in definitions section

aa3b67e

worker builds

41a18ff

scheduler builds

00e9dbd

destinations

329b0f1

java sources

ef4aa26

python?

4394f0f

the ones we left behind

c0af4da

remove unused

188b307

comment

b95ae2a

add test for new jsonschema fetching

1c8982c

bump version of connectors used in acceptance tests

ca71cc8

bump versions

f0727ae

check images early

cb839c4

bump the chimp

29692b4

Revert "bump the chimp"

da059f0

This reverts commit 29692b4.

handle stream name fixes

80d956a

cgardens force-pushed the cgardens/incremental_model branch from 1b0b7c0 to 80d956a Compare November 18, 2020 21:40

clean

697bdbe

cgardens merged commit e7edb2c into master Nov 18, 2020

cgardens deleted the cgardens/incremental_model branch November 18, 2020 22:16

cgardens mentioned this pull request Nov 19, 2020

Configuration Data model for Incremental in AirbyteCatalog #988

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding incremental to the data model #998

Adding incremental to the data model #998

cgardens commented Nov 16, 2020 •

edited

Loading

jrhizor left a comment

sherifnada left a comment

michel-tricot Nov 17, 2020

cgardens Nov 17, 2020

cgardens Nov 17, 2020

cgardens Nov 17, 2020 •

edited

Loading

cgardens Nov 18, 2020

michel-tricot commented Nov 17, 2020 via email

michel-tricot commented Nov 17, 2020 via email

cgardens commented Nov 17, 2020 •

edited

Loading

cgardens Nov 18, 2020

cgardens Nov 18, 2020

cgardens Nov 18, 2020

michel-tricot Nov 18, 2020

michel-tricot commented Nov 18, 2020

cgardens commented Nov 18, 2020

		@@ -0,0 +1,22 @@
		# Configuration: Streams & Incremental Syncs

		## AirbyteStream

Adding incremental to the data model #998

Adding incremental to the data model #998

Conversation

cgardens commented Nov 16, 2020 • edited Loading

What

Checklist

Misc

Please no more requests for name changes in AirbyteCatalog and AirbyteStream

jrhizor left a comment

Choose a reason for hiding this comment

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

cgardens Nov 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michel-tricot commented Nov 17, 2020 via email

michel-tricot commented Nov 17, 2020 via email

cgardens commented Nov 17, 2020 • 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

michel-tricot commented Nov 18, 2020

cgardens commented Nov 18, 2020

cgardens commented Nov 16, 2020 •

edited

Loading

cgardens Nov 17, 2020 •

edited

Loading

cgardens commented Nov 17, 2020 •

edited

Loading