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

ChristopheDuong · 2021-03-03T19:22:28Z

What

Introduce new prefix field on the sync page to use as prefix for destination tables

Implements #2239
(see #2335 too)

How

Describe the solution
I introduced a new airbyte-workers/src/main/java/io/airbyte/workers/protocols/airbyte/AirbyteMapper.java hierarchy class to mimic the same as Source and Destination

This Is passed to the DefaultSyncWorker constructor and does only one transformation about the namespace field for the moment.
It mutates the catalog object config before passing it to the AirbyteDestination but doesn't affect the catalog from the source

Pre-merge Checklist

Run integration tests
Publish Docker images

Recommended reading order

airbyte-workers/src/main/java/io/airbyte/workers/protocols/airbyte/DefaultAirbyteMapper.java
airbyte-api/src/main/openapi/config.yaml
the rest

cgardens · 2021-03-03T19:29:05Z

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

        - sourceId
        - destinationId
        - status
      properties:
        name:
          type: string
          description: Optional name of the connection
+        connectionName:


Why not just called it prefix? tableNamePrefix? The connection already has a name. You're adding a field to allow prefixing right? Maybe I'm missing some bigger picture here, but it seems like you could use a really specific clean name here instead of something broad.

in the short term, it's a prefix but in one or two weeks, when namespace fields are available, this field becomes the actual namespace name for the destination.

On top of that destination namespace name, you can have an optional suffix namespace whenever the source supports it (in JDBC source connectors) but in all other cases, the suffix will be null.

To conclude, the connectionName can't be empty when used with non-JDBC sources.

IMO: This is very complex & we should call things by their names.

if it is a prefix we should call it prefix.
if it is a default namespace for record which doesn't have a namespace, then we should call it defaultNamespace

ConnectionName seems like a dead field at that point.

i just realized I called it namespaceDefault instead of defaultNamespace... I hope that's fine...

in the short term, it's a prefix but in one or two weeks, when namespace fields are available, this field becomes the actual namespace name for the destination.

in that second step, how will this work with non database sources that declare namespaces? if this a default and gets overridden by the namespace set by the source, won't tables in the destination still collide? e.g. if you were pulling from 2 stripe sources both will try to write to stripe.users? if it's an override then it will still work, but if it's a default then i think they collide? 🤔 maybe i'm missing something.

i'm still a little unsure if this a namespace default or namespace override. I'm a little fuzzy on what's going to happen if this value is not set. it looks like below we are going throw an NPE or end up with tables that are null_users. maybe i'm missing something? anyway, if you have a plan i don't want to derail it, but i'm having a little trouble figuring out how this is going to fit together.

Normally, the default namespace should always be defined to specify where data will end up in the destination.

The only case where it would be acceptable for it to be null is when the stream already has a namespace defined by the source (thus requires a namespace field introduced by #2228) AND when the user wants to mirror exactly the same namespace structure in the destination as in the source (replicate public.users from source in public.users in destination)

But for the short term, since destinations are still set with one (and it is required), it is not currently mandatory here immediately. (I will handle the NPE though)

in that second step, how will this work with non database sources that declare namespaces?

let's move that thread discussion over there where I will detail more what we do in the next steps.

#2324

…-stream-name

ChristopheDuong · 2021-03-04T16:33:32Z

airbyte-workers/src/main/java/io/airbyte/workers/WorkerConstants.java

@@ -29,7 +29,8 @@
  public static final String TAP_CONFIG_JSON_FILENAME = "tap_config.json";
  public static final String TARGET_CONFIG_JSON_FILENAME = "target_config.json";

-  public static final String CATALOG_JSON_FILENAME = "catalog.json";
+  public static final String TAP_CATALOG_JSON_FILENAME = "tap_catalog.json";


I've separated the catalog in two versions for the source/destination...

BTW should we rename these constants to

source_catalog.json / destination_catalog.json?

source_config.json / destination_config.json?

Sounds good to me!

If you do so, also rename the const names

ChristopheDuong · 2021-03-04T18:03:02Z

I'm planning to add a few comments, look at acceptance tests and add a test class for the airbyte-workers/src/main/java/io/airbyte/workers/protocols/airbyte/DefaultAirbyteMapper.java class that I introduced here but it should be ready for review in the meantime.

michel-tricot

% suggested changes

airbyte-workers/src/main/java/io/airbyte/workers/protocols/Mapper.java

michel-tricot · 2021-03-04T18:27:30Z

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

@@ -1595,13 +1595,17 @@ components:
    ConnectionCreate:
      type: object
      required:
+        - namespaceDefault


It is more correct if it is called defaultNamespace

michel-tricot · 2021-03-04T18:31:25Z

airbyte-workers/src/main/java/io/airbyte/workers/WorkerConstants.java

@@ -29,7 +29,8 @@
  public static final String TAP_CONFIG_JSON_FILENAME = "tap_config.json";
  public static final String TARGET_CONFIG_JSON_FILENAME = "target_config.json";

-  public static final String CATALOG_JSON_FILENAME = "catalog.json";
+  public static final String TAP_CATALOG_JSON_FILENAME = "tap_catalog.json";


Sounds good to me!

michel-tricot · 2021-03-04T18:34:04Z

airbyte-workers/src/main/java/io/airbyte/workers/WorkerConstants.java

@@ -29,7 +29,8 @@
  public static final String TAP_CONFIG_JSON_FILENAME = "tap_config.json";
  public static final String TARGET_CONFIG_JSON_FILENAME = "target_config.json";

-  public static final String CATALOG_JSON_FILENAME = "catalog.json";
+  public static final String TAP_CATALOG_JSON_FILENAME = "tap_catalog.json";


If you do so, also rename the const names

airbyte-workers/src/main/java/io/airbyte/workers/protocols/airbyte/AirbyteMapper.java

...te-workers/src/main/java/io/airbyte/workers/protocols/airbyte/DefaultAirbyteDestination.java

airbyte-workers/src/main/java/io/airbyte/workers/protocols/airbyte/DefaultAirbyteMapper.java

ChristopheDuong · 2021-03-04T20:18:26Z

airbyte-workers/src/main/java/io/airbyte/workers/protocols/airbyte/NamespacingMapper.java

+
+  @Override
+  public AirbyteMessage apply(final AirbyteMessage inputMessage) {
+    final AirbyteMessage message = Jsons.clone(inputMessage);


Is the clone operation really needed here? (but then we would mutate the inputMessage as a side-effect.

In the case of the catalog, the original catalog is still in use by the source so that's good to do a clone.

In terms of performances, it would be avoiding a copy of messages since the original message is not used anyway?
only the transformed one is passed to the destination

probably doing the clone is safest for now to avoid any confusion. especially if we end up chaining these, i think it would confusing if they are mutating in place. but you're right there is some performance cost.

cgardens · 2021-03-04T21:15:56Z

airbyte-workers/src/main/java/io/airbyte/workers/WorkerConstants.java

@@ -29,7 +29,8 @@
  public static final String TAP_CONFIG_JSON_FILENAME = "tap_config.json";
  public static final String TARGET_CONFIG_JSON_FILENAME = "target_config.json";

-  public static final String CATALOG_JSON_FILENAME = "catalog.json";
+  public static final String TAP_CATALOG_JSON_FILENAME = "tap_catalog.json";


cgardens · 2021-03-04T21:20:28Z

airbyte-workers/src/main/java/io/airbyte/workers/protocols/airbyte/NamespacingMapper.java

+
+  @Override
+  public AirbyteMessage apply(final AirbyteMessage inputMessage) {
+    final AirbyteMessage message = Jsons.clone(inputMessage);


probably doing the clone is safest for now to avoid any confusion. especially if we end up chaining these, i think it would confusing if they are mutating in place. but you're right there is some performance cost.

cgardens · 2021-03-04T21:22:31Z

airbyte-workers/src/main/java/io/airbyte/workers/protocols/airbyte/NamespacingMapper.java

+    if (message.getCatalog() != null) {
+      message.getCatalog().getStreams().forEach(s -> mutateStream(s, defaultNamespace));
+    }
+    if (message.getRecord() != null) {


I think you can do this instead, which is a bit more readable.

Suggested change

if (message.getRecord() != null) {

if (message.getType() == Type.RECORD) {

cgardens · 2021-03-04T21:23:20Z

airbyte-workers/src/main/java/io/airbyte/workers/protocols/airbyte/NamespacingMapper.java

+  @Override
+  public AirbyteMessage apply(final AirbyteMessage inputMessage) {
+    final AirbyteMessage message = Jsons.clone(inputMessage);
+    if (message.getCatalog() != null) {


when would the sync worker ever be consuming catalog messages? unless there's a specific case i think we can remove this conditional.

cgardens · 2021-03-04T21:25:58Z

airbyte-workers/src/main/java/io/airbyte/workers/protocols/Mapper.java

+
+  StandardTargetConfig apply(StandardTargetConfig targetConfig);
+
+  T apply(T message);


can we have more explicit names for these methods. having them both be the same and just called apply is hard to parse. maybe mapCatalog and mapMessage?

cgardens · 2021-03-04T21:26:59Z

airbyte-workers/src/main/java/io/airbyte/workers/protocols/Mapper.java

+
+public interface Mapper<T> {
+
+  StandardTargetConfig apply(StandardTargetConfig targetConfig);


why is this taking in StandardTargetConfig and opposed to ConfiguredAirbyteCatalog? The point is to map one catalog to another, right? StandardTargetConfig doesn't really have anything to do with it; it just happens to be a containing object.

cgardens · 2021-03-04T21:45:06Z

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

        - sourceId
        - destinationId
        - status
      properties:
        name:
          type: string
          description: Optional name of the connection
+        connectionName:


in the short term, it's a prefix but in one or two weeks, when namespace fields are available, this field becomes the actual namespace name for the destination.

in that second step, how will this work with non database sources that declare namespaces? if this a default and gets overridden by the namespace set by the source, won't tables in the destination still collide? e.g. if you were pulling from 2 stripe sources both will try to write to stripe.users? if it's an override then it will still work, but if it's a default then i think they collide? 🤔 maybe i'm missing something.

cgardens · 2021-03-04T21:47:17Z

airbyte-workers/src/main/java/io/airbyte/workers/protocols/airbyte/NamespacingMapper.java

+  private final String defaultNamespace;
+
+  public NamespacingMapper(String defaultNamespace) {
+    this.defaultNamespace = defaultNamespace;


what happens if the default namespace is null?

cgardens · 2021-03-04T21:51:31Z

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

        - sourceId
        - destinationId
        - status
      properties:
        name:
          type: string
          description: Optional name of the connection
+        connectionName:


i'm still a little unsure if this a namespace default or namespace override. I'm a little fuzzy on what's going to happen if this value is not set. it looks like below we are going throw an NPE or end up with tables that are null_users. maybe i'm missing something? anyway, if you have a plan i don't want to derail it, but i'm having a little trouble figuring out how this is going to fit together.

cgardens

nice! some questions around readability but i think they are all straight forward. some bigger questions in the existing thread here

…e-to-stream-name

* Add connectionName * Change connectionName to namespaceDefault * run get_spec on temporal (#2299) Co-authored-by: Charles <[email protected]>

ChristopheDuong · 2021-03-05T11:03:54Z

@johnlafleur I also made a short description in video for the changelog here https://www.loom.com/share/cc3cca41c37549779381abd847f2a003

Add connection name to Standard Sync

1a14cc7

ChristopheDuong mentioned this pull request Mar 3, 2021

Add a field to input a sync/connection name #2285

Closed

cgardens reviewed Mar 3, 2021

View reviewed changes

ChristopheDuong added 4 commits March 3, 2021 23:46

Add to webbackend too

3e8fa31

Rename connectionName to namespaceDefault

2e13046

Merge remote-tracking branch 'origin/master' into inject-sync-name-to…

b2d9819

…-stream-name

Add Transformations

8357e2c

ChristopheDuong force-pushed the chris/inject-sync-name-to-stream-name branch from f059ee5 to 8357e2c Compare March 4, 2021 14:31

ChristopheDuong changed the title ~~Add connection name to Standard Sync~~ Add namespaceDefault to destination stream name Mar 4, 2021

Persist catalogs for source and destinations separately

dbe2ecf

ChristopheDuong commented Mar 4, 2021

View reviewed changes

ChristopheDuong added 2 commits March 4, 2021 18:28

Fix Update

3008509

Fix webbackendread

2915ea5

ChristopheDuong marked this pull request as ready for review March 4, 2021 18:03

ChristopheDuong requested review from michel-tricot, cgardens, jamakase, sherifnada and jrhizor March 4, 2021 18:03

ChristopheDuong changed the title ~~Add namespaceDefault to destination stream name~~ ☝🏼Add new namespaceDefault field in connection page to destination stream name (as a prefix to the table name) Mar 4, 2021

Fix acceptance tests

7e0e1d9

michel-tricot approved these changes Mar 4, 2021

View reviewed changes

ChristopheDuong added 2 commits March 4, 2021 20:30

Tweak from code review

c6e4fdd

Fix Tests

5369900

ChristopheDuong commented Mar 4, 2021

View reviewed changes

cgardens reviewed Mar 4, 2021

View reviewed changes

cgardens approved these changes Mar 4, 2021

View reviewed changes

jamakase mentioned this pull request Mar 4, 2021

Jamakase/add connection name field #2311

Merged

ChristopheDuong mentioned this pull request Mar 5, 2021

UI changes around namespacing stream names #2324

Closed

ChristopheDuong and others added 5 commits March 5, 2021 10:45

Unit tests and rename constants

be044e3

Merge remote-tracking branch 'origin/mt-fix-dev' into inject-sync-nam…

0ee819d

…e-to-stream-name

Jamakase/add connection name field (#2311)

7629baa

* Add connectionName * Change connectionName to namespaceDefault * run get_spec on temporal (#2299) Co-authored-by: Charles <[email protected]>

Rename namespaceDefault to defaultNamespace

25dcf78

Remove underscore

9cb082b

Fix tests

0decc8a

ChristopheDuong merged commit 20dec48 into master Mar 5, 2021

ChristopheDuong deleted the chris/inject-sync-name-to-stream-name branch March 5, 2021 11:54

This was referenced Mar 5, 2021

Migration to make sure defaultNamespace is set #2329

Closed

Namespace destination output #1921

Closed

ChristopheDuong changed the title ~~☝🏼Add new namespaceDefault field in connection page to destination stream name (as a prefix to the table name)~~ ☝🏼Add new prefix field in connection page (prepended to the stream name) Mar 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ChristopheDuong commented Mar 3, 2021 •

edited

Loading

cgardens Mar 3, 2021

ChristopheDuong Mar 3, 2021

michel-tricot Mar 4, 2021

ChristopheDuong Mar 4, 2021 •

edited

Loading

cgardens Mar 4, 2021 •

edited

Loading

cgardens Mar 4, 2021 •

edited

Loading

ChristopheDuong Mar 5, 2021 •

edited

Loading

ChristopheDuong Mar 5, 2021

ChristopheDuong Mar 4, 2021 •

edited

Loading

michel-tricot Mar 4, 2021

michel-tricot Mar 4, 2021

cgardens Mar 4, 2021

ChristopheDuong commented Mar 4, 2021 •

edited

Loading

michel-tricot left a comment

michel-tricot Mar 4, 2021

michel-tricot Mar 4, 2021

michel-tricot Mar 4, 2021

ChristopheDuong Mar 4, 2021 •

edited

Loading

cgardens Mar 4, 2021

cgardens Mar 4, 2021

cgardens Mar 4, 2021

cgardens Mar 4, 2021

cgardens Mar 4, 2021

cgardens Mar 4, 2021

cgardens Mar 4, 2021

cgardens Mar 4, 2021 •

edited

Loading

cgardens Mar 4, 2021

cgardens Mar 4, 2021 •

edited

Loading

cgardens left a comment

ChristopheDuong commented Mar 5, 2021 •

edited

Loading

	if (message.getRecord() != null) {
	if (message.getType() == Type.RECORD) {


		StandardTargetConfig apply(StandardTargetConfig targetConfig);

		T apply(T message);


		public interface Mapper<T> {

		StandardTargetConfig apply(StandardTargetConfig targetConfig);

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

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

Conversation

ChristopheDuong commented Mar 3, 2021 • edited Loading

What

How

Pre-merge Checklist

Recommended reading order

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChristopheDuong Mar 4, 2021 • edited Loading

Choose a reason for hiding this comment

cgardens Mar 4, 2021 • edited Loading

Choose a reason for hiding this comment

cgardens Mar 4, 2021 • edited Loading

Choose a reason for hiding this comment

ChristopheDuong Mar 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChristopheDuong Mar 4, 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

ChristopheDuong commented Mar 4, 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 4, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgardens Mar 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgardens Mar 4, 2021 • edited Loading

Choose a reason for hiding this comment

cgardens left a comment

Choose a reason for hiding this comment

ChristopheDuong commented Mar 5, 2021 • edited Loading

ChristopheDuong commented Mar 3, 2021 •

edited

Loading

ChristopheDuong Mar 4, 2021 •

edited

Loading

cgardens Mar 4, 2021 •

edited

Loading

cgardens Mar 4, 2021 •

edited

Loading

ChristopheDuong Mar 5, 2021 •

edited

Loading

ChristopheDuong Mar 4, 2021 •

edited

Loading

ChristopheDuong commented Mar 4, 2021 •

edited

Loading

ChristopheDuong Mar 4, 2021 •

edited

Loading

cgardens Mar 4, 2021 •

edited

Loading

cgardens Mar 4, 2021 •

edited

Loading

ChristopheDuong commented Mar 5, 2021 •

edited

Loading