-
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
Destination ClickHouse: remove tcp port spec parameter #16970
Conversation
airbyte-config/init/src/main/resources/seed/destination_definitions.yaml
Outdated
Show resolved
Hide resolved
Thanks for the contribution @mshustov someone from the team will review it later this week or next one. |
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.
LGTM.
@mshustov There are several small things that need to happen before this PR can be published and merged:
- update version in Dockerfile in destination-clickhouse and destination-clickhouse-strict-encrypt to 0.2.0
- add a line to the change log in clickhouse.md
- remove changes from YAML files in seed folder (these will be updated by publish command)
@marcosmarxm @grishick done! |
/test connector=connectors/destination-clickhouse
Build FailedTest summary info:
|
I think there is one more change that is needed here to fix the failing unit test. I'll am testing the theory here: #17483 |
I ended up adding two more changes to fix the normalization tests, publishing the new versions and merging these changes in this pr. I |
What
ClickHouse configuration in Airbyte requires both HTTP and Native port to be specified.
The HTTP port is used for JDBC driver to communicate with ClickHouse, the native port is used by dbt-clickhouse during transformation step.
The native port is not a requirement for DBT-clickhouse anymore since the last the last versions of dbt-clickhouse support both HTTP and native protocols. We want to switch to HTTP by default everywhere since it reduces the technical complexity of the integration.
How
This PR removes requirements for Native port to be provided to reduce the configuration size and improve UX. I removed all the mentions of TCP port in the codebase introduced in #9340 and #14783
Recommended reading order
*.yml
files🚨 User Impact 🚨
The number of required fields in ClickHouse configuration was reduced. I didn't manage to run Airbyte locally (neither master branch nor the current one) to take a screenshot. In short, it should remove
native port
field:SUB_BUILD=PLATFORM ./gradlew build
fails withPre-merge Checklist
Expand the relevant checklist and delete the others.
New Connector
Community member or Airbyter
airbyte_secret
./gradlew :airbyte-integrations:connectors:<name>:integrationTest
.README.md
bootstrap.md
. See description and examplesdocs/integrations/<source or destination>/<name>.md
including changelog. See changelog exampledocs/integrations/README.md
airbyte-integrations/builds.md
Airbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
/test connector=connectors/<name>
command is passing/publish
command described hereUpdating a connector
Community member or Airbyter
airbyte_secret
./gradlew :airbyte-integrations:connectors:<name>:integrationTest
.README.md
bootstrap.md
. See description and examplesdocs/integrations/<source or destination>/<name>.md
including changelog. See changelog exampleAirbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
/test connector=connectors/<name>
command is passing/publish
command described hereConnector Generator
-scaffold
in their name) have been updated with the latest scaffold by running./gradlew :airbyte-integrations:connector-templates:generator:testScaffoldTemplates
then checking in your changesTests
Unit
Put your unit tests output here.
Integration
Put your integration tests output here.
Acceptance
Put your acceptance tests output here.