Step 2: Add Namespace to Airbyte Protocol #2228

ChristopheDuong · 2021-02-26T17:04:29Z

What

Describe what the change is solving
Same as #1993 to implement #1921

How

Describe the solution

This introduces a new object AirbyteStreamName in both the API and the Airbyte Protocol to represent namespace and table name in a single struct.
The old name of AirbyteStream is kept as it was but the new fields are properly populated with separate namespace/table names.

So for example, we would have catalog produced by the JDBC sources:

Stream name: public.users (like before)
AirbyteStreamName.namespace: public (new)
AirbyteStreamName.name: users (new)

Non-jdbc sources are now producing:

Stream name: users (like before)
AirbyteStreamName.namespace: (empty)
AirbyteStreamName.name: users (same as stream name)

These new fields won't be used right away so they shouldn't introduce any change to the product until the last step is implemented.

Pre-merge Checklist

Run integration tests
Publish Docker images
Verify Acceptance tests (depends on new docker images)

Recommended reading order

airbyte-api/src/main/openapi/config.yaml
airbyte-protocol/models/src/main/resources/airbyte_protocol/airbyte_protocol.yaml
airbyte-server/src/main/java/io/airbyte/server/converters/CatalogConverter.java
airbyte-integrations/connectors/source-jdbc/src/main/java/io/airbyte/integrations/source/jdbc/AbstractJdbcSource.java
the rest

ChristopheDuong · 2021-02-26T17:07:49Z

When we make changes to the Airbyte Protocol, old source connectors are incompatible with newer versions of the protocol:

If I run a connector with a version from before my change to the protocol where I added the stream_name field, the validation of the Catalog from the old connector code will fail:

Caused by: com.fasterxml.jackson.databind.exc.UnrecognizedPropertyException: Unrecognized field "stream_name" (class io.airbyte.protocol.models.AirbyteStream), not marked as ignorable (5 known properties: "json_schema", "name", "supported_sync_modes", "default_cursor_field", "source_defined_cursor"])

logs-7-0.txt

How should I deal with this?

Make jsons validations in connectors a little more permissive on unknown properties. Then force bump/upgrade all source connectors via a migration?
Strip/back-migrate catalogs back to the proper protocol version for each connector

michel-tricot

Looks really cool!

...t-integration/java/io/airbyte/integrations/destination/bigquery/BigQueryDestinationTest.java

airbyte-protocol/models/src/main/java/io/airbyte/protocol/models/CatalogHelpers.java

michel-tricot · 2021-02-26T23:40:11Z

airbyte-server/src/main/java/io/airbyte/server/converters/CatalogConverter.java

@@ -34,9 +34,34 @@
 */
 public class CatalogConverter {

+  private static io.airbyte.api.model.AirbyteStreamName toApi(final String name, final io.airbyte.protocol.models.AirbyteStreamName streamName) {


streamname should be the first parameter since it is the one that will remain.

Can you add a Preconditions that checks that only one of the two is set? (assuming this is correct)

for the moment both are set, sometimes the StreamName object could be null (if it was serialized before introducing the namespace field)

airbyte-server/src/main/java/io/airbyte/server/converters/CatalogConverter.java

michel-tricot · 2021-02-26T23:42:35Z

airbyte-tests/src/acceptanceTests/java/io/airbyte/test/acceptance/AcceptanceTests.java

@@ -281,6 +283,7 @@ public void testDiscoverSourceSchema() throws ApiException {
        .build());
    final AirbyteStream stream = new AirbyteStream()
        .name(STREAM_NAME)


should we unset this?

No, this is required as we are currently backward compatible and everything keeps working as it was before.

The new AirbyteStreamName field is not actually influencing the stream names for the moment

I created this issue to remove such methods usage here though: #2240

...yte-workers/src/test/java/io/airbyte/workers/protocols/airbyte/DefaultAirbyteSourceTest.java

…AM_NAMESPACE, STREAM_NAME)

michel-tricot

Looks good! can you also get a review from someone else in the team before you merge?

michel-tricot · 2021-03-04T03:51:42Z

...nectors/source-mssql/src/test/java/io/airbyte/integrations/source/mssql/MssqlSourceTest.java

-  private static final String STREAM_NAME = "dbo.id_and_name";
+  private static final String STREAM_NAMESPACE = "dbo";
+  private static final String TABLE_NAME = "id_and_name";
+  private static final String STREAM_NAME = STREAM_NAMESPACE + "." + TABLE_NAME;


shouldn't the stream name just be tablename?

The previous STREAM_NAME was "dbo.id_and_name"

We didn't introduce big changes on how things work so the stream name is still the same as before (but now it is also just "parsed" and split in two in the new AirbyteStreamName struct)

michel-tricot · 2021-03-04T03:54:08Z

airbyte-server/src/main/java/io/airbyte/server/converters/CatalogConverter.java

@@ -79,10 +123,15 @@
    final List<io.airbyte.api.model.AirbyteStreamAndConfiguration> streams = catalog.getStreams()
        .stream()
        .map(configuredStream -> {
+          final String streamName;
+          if (configuredStream.getStream().getStreamName() != null)


NIT You're missing {

michel-tricot · 2021-03-04T03:55:08Z

airbyte-server/src/main/java/io/airbyte/server/converters/CatalogConverter.java

@@ -79,10 +123,15 @@
    final List<io.airbyte.api.model.AirbyteStreamAndConfiguration> streams = catalog.getStreams()
        .stream()
        .map(configuredStream -> {
+          final String streamName;


this variable name is super confusing. I think you should just call it name

cgardens

this seems like it should work okay (as long as all of the tests for the jdbc sources are passing).

I am not convinced that adding the AirbyteStreamName adds anything over just adding a namespace field. It seems like the extra object is kinda a headache. I think it would also be easier from a backwards compatibility point of view.

airbyte-api/src/main/openapi/config.yaml

cgardens · 2021-03-04T06:34:35Z

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

@@ -127,6 +127,9 @@ definitions:
    properties:
      name:
        type: string
+        description: Stream's name (deprecated, will be replaced by stream_name).
+      stream_name:


would it be simpler if instead of adding this AirbyteStreamName object, you just added namespace as a property to this top level object? then we don't need to deprecate name? it also saves us having to ever have name and stream_name at the same time.

i got to think it makes the backwards compatibility story way simpler too.

in my last PR, @michel-tricot was mentioning this: #1993 (comment)

Did I misunderstand his comment?

kk. i don't think you misunderstood. up to you.

cgardens · 2021-03-04T06:36:49Z

...ectors/source-jdbc/src/main/java/io/airbyte/integrations/source/jdbc/AbstractJdbcSource.java

@@ -134,7 +134,9 @@ public AirbyteCatalog discover(JsonNode config) throws Exception {
              Optional.ofNullable(config.get("database")).map(JsonNode::asText),
              Optional.ofNullable(config.get("schema")).map(JsonNode::asText))
                  .stream()
-                  .map(t -> CatalogHelpers.createAirbyteStream(t.getName(), t.getFields())
+                  .map(t -> CatalogHelpers.createAirbyteStream(CatalogHelpers.createAirbyteStreamName(t.getSchemaName(), t.getName()), t.getFields())
+                      // TODO: Switch fully to StreamName instead of temporarily setName() for backward compatibility


can you add your github user name, or something else identifying when you add todos?

cgardens · 2021-03-04T06:43:59Z

...ectors/source-jdbc/src/main/java/io/airbyte/integrations/source/jdbc/AbstractJdbcSource.java

    private final String name;
    private final List<Field> fields;

-    public TableInfo(String name, List<Field> fields) {
+    public TableInfo(String schemaName, String name, List<Field> fields) {


does this all work okay with MySQL (which doesn't have schemas)?

Yes, I think we already figured it out when we made the PR to discover tables from another schema than just the public one.

cgardens · 2021-03-04T07:22:24Z

what i'm describing won't work, because you're changing behavior in the sources before the destinations are compatible with namespaces so you're a little boxed. if you were just doing destinations first, i think you could the in place replacement (i.e. just add namespace). in that process each destination would be made to support namespaces first when they were present. each one could be migrated. once that was done, we could start migrating sources, and as they had non null values for namespaces the destinations would respect them). i don't think i love having this extra object, but it would have to be a pretty execution plan to get around it.

probably fine to keep it as is, but wanted to at least offer this different perspective.

Co-authored-by: Charles <giardina.charles@gmail.com>

ChristopheDuong added 3 commits February 26, 2021 17:48

Add AirbyteStreamName to include namespace fields

b17c859

Convert namespace fields between api and protocol

e1ded48

Fix tests using new namespace field

c74b525

ChristopheDuong changed the title ~~Add Namespace to Airbyte Protocol~~ Step 1/3: Add Namespace to Airbyte Protocol Feb 26, 2021

ChristopheDuong mentioned this pull request Feb 26, 2021

Namespace destination output #1921

Closed

michel-tricot reviewed Feb 26, 2021

View reviewed changes

ChristopheDuong mentioned this pull request Mar 1, 2021

☝🏼 Upgrade all connectors (0.2.0) so protocol allows future / unknown properties #2238

Merged

3 tasks

ChristopheDuong added this to the 2021-03-05 milestone Mar 1, 2021

ChristopheDuong self-assigned this Mar 1, 2021

ChristopheDuong mentioned this pull request Mar 1, 2021

Connectors should use the StreamName namespace & name fields #2240

Closed

Create new helper method: CatalogHelpers.createAirbyteStreamName(STRE…

7269de2

…AM_NAMESPACE, STREAM_NAME)

ChristopheDuong marked this pull request as ready for review March 2, 2021 19:57

ChristopheDuong requested review from michel-tricot, cgardens, sherifnada and jrhizor March 3, 2021 15:14

michel-tricot approved these changes Mar 4, 2021

View reviewed changes

cgardens approved these changes Mar 4, 2021

View reviewed changes

Update airbyte-api/src/main/openapi/config.yaml

fc86bf9

Co-authored-by: Charles <giardina.charles@gmail.com>

ChristopheDuong mentioned this pull request Mar 5, 2021

☝🏼Add new prefix field in connection page (prepended to the stream name) #2298

Merged

2 tasks

ChristopheDuong changed the title ~~Step 1/3: Add Namespace to Airbyte Protocol~~ Step 2: Add Namespace to Airbyte Protocol Mar 5, 2021

cgardens modified the milestones: Core - 2021-03-05, Core - 2021-03-12 Mar 8, 2021

ChristopheDuong closed this May 3, 2021

swyxio deleted the chris/namespace-protocol branch October 11, 2022 15:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Step 2: Add Namespace to Airbyte Protocol #2228

Step 2: Add Namespace to Airbyte Protocol #2228

ChristopheDuong commented Feb 26, 2021 •

edited

Loading

ChristopheDuong commented Feb 26, 2021 •

edited

Loading

michel-tricot left a comment

michel-tricot Feb 26, 2021

ChristopheDuong Mar 1, 2021

michel-tricot Feb 26, 2021

ChristopheDuong Mar 1, 2021 •

edited

Loading

michel-tricot left a comment

michel-tricot Mar 4, 2021

ChristopheDuong Mar 5, 2021

michel-tricot Mar 4, 2021

michel-tricot Mar 4, 2021

cgardens left a comment

cgardens Mar 4, 2021

cgardens Mar 4, 2021

ChristopheDuong Mar 4, 2021

cgardens Mar 5, 2021 •

edited

Loading

cgardens Mar 4, 2021

cgardens Mar 4, 2021

ChristopheDuong Mar 5, 2021

cgardens Mar 5, 2021

cgardens commented Mar 4, 2021

Step 2: Add Namespace to Airbyte Protocol #2228

Step 2: Add Namespace to Airbyte Protocol #2228

Conversation

ChristopheDuong commented Feb 26, 2021 • edited Loading

What

How

Pre-merge Checklist

Recommended reading order

ChristopheDuong commented Feb 26, 2021 • edited Loading

michel-tricot 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

ChristopheDuong Mar 1, 2021 • edited Loading

Choose a reason for hiding this comment

michel-tricot 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

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

Choose a reason for hiding this comment

cgardens Mar 5, 2021 • 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

cgardens commented Mar 4, 2021

ChristopheDuong commented Feb 26, 2021 •

edited

Loading

ChristopheDuong commented Feb 26, 2021 •

edited

Loading

ChristopheDuong Mar 1, 2021 •

edited

Loading

cgardens Mar 5, 2021 •

edited

Loading