-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Namespace fields in catalog #1993
Conversation
e8de614
to
fa26e08
Compare
With this PR: As a result of this protocol change, It seems the code base of core server & destination connectors using this new protocol are not backward compatible anymore with source connectors
Example of logs if I try to setup
It seems the catalog.json passed to the source with the "old protocol" when reading during sync is rejected because of the new fields used by destinations |
airbyte-integrations/connectors/source-google-sheets/unit_tests/test_helpers.py
Outdated
Show resolved
Hide resolved
...-jdbc/src/test-integration/java/io/airbyte/integrations/source/jdbc/JdbcIntegrationTest.java
Outdated
Show resolved
Hide resolved
...tors/source-jdbc/src/test/java/io/airbyte/integrations/source/jdbc/IncrementalUtilsTest.java
Outdated
Show resolved
Hide resolved
.../src/test-integration/java/io/airbyte/integrations/source/mssql/MssqlSourceStandardTest.java
Outdated
Show resolved
Hide resolved
...io/airbyte/integrations/io/airbyte/integration_tests/sources/RedshiftStandardSourceTest.java
Outdated
Show resolved
Hide resolved
airbyte-protocol/models/src/main/resources/airbyte_protocol/airbyte_protocol.yaml
Outdated
Show resolved
Hide resolved
@@ -171,6 +176,14 @@ definitions: | |||
type: array | |||
items: | |||
type: string | |||
alias_name: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why did you decide to drop the destination and go for alias & target? I have a preference for the destination prefix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No particular reason, I reflected on it and thought it would make it clearer what it is used for afterward... but we can always revert to destination_name
/ destination_namespace
instead If you think it's better
@@ -144,6 +144,9 @@ definitions: | |||
type: array | |||
items: | |||
type: string | |||
namespace: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you move it close to name since these two work together?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should create an object that contains both name & namespace that we can use for both the stream and the configured stream.
WDYT?
StreamName {
string: name
string: namespace
}
What
Describe what the change is solving
Implements #1921
(blocked/depends on #1934)
How
Describe the solution
Add namespace field to Catalog (optional) and ConfiguredCatalog (mandatory) to specify where destination should write the final table.
Pre-merge Checklist
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