Skip to content
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.

Sign up for GitHub

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

Jump to bottom

grab bag of non-controversial clean up tasks #26702

Merged
merged 3 commits into from
May 31, 2023
Merged

grab bag of non-controversial clean up tasks #26702

merged 3 commits into from
May 31, 2023

Conversation

cgardens
Copy link
Contributor

What

  • Various clean up that we did on the way for async snowflake. Pulling into its own PR to keep the harder PRs more focused.

@cgardens cgardens requested review from a team as code owners May 27, 2023 07:24
@github-actions
Copy link
Contributor

github-actions bot commented May 27, 2023 •
edited
Loading

Affected Connector Report

NOTE ⚠️ Changes in this PR affect the following connectors. Make sure to do the following as needed:

  • Run integration tests
  • Bump connector or module version
  • Add changelog
  • Publish the new version

✅ Sources (33)
Connector Version Changelog Publish
source-alloydb 2.0.28 ✅ ✅
source-alloydb-strict-encrypt 2.0.28 🔵
(ignored)		 🔵
(ignored)
source-azure-blob-storage 0.1.0 ✅ ✅
source-bigquery 0.2.3 ✅ ✅
source-clickhouse 0.1.17 ✅ ✅
source-clickhouse-strict-encrypt 0.1.17 🔵
(ignored)		 🔵
(ignored)
source-cockroachdb 0.1.22 ✅ ✅
source-cockroachdb-strict-encrypt 0.1.22 🔵
(ignored)		 🔵
(ignored)
source-db2 0.1.19 ✅ ✅
source-db2-strict-encrypt 0.1.19 🔵
(ignored)		 🔵
(ignored)
source-dynamodb 0.1.2 ✅ ✅
source-e2e-test 2.1.4 ✅ ✅
source-e2e-test-cloud 2.1.4 🔵
(ignored)		 🔵
(ignored)
source-elasticsearch 0.1.1 ✅ ✅
source-jdbc 0.3.5 🔵
(ignored)		 🔵
(ignored)
source-kafka 0.2.3 ✅ ✅
source-mongodb-strict-encrypt 0.1.19 🔵
(ignored)		 🔵
(ignored)
source-mongodb-v2 0.1.19 ✅ ✅
source-mssql 1.0.17 ✅ ✅
source-mssql-strict-encrypt 1.0.17 🔵
(ignored)		 🔵
(ignored)
source-mysql 2.0.24 ✅ ✅
source-mysql-strict-encrypt 2.0.24 🔵
(ignored)		 🔵
(ignored)
source-oracle 0.3.24 ✅ ✅
source-oracle-strict-encrypt 0.3.24 🔵
(ignored)		 🔵
(ignored)
source-postgres 2.0.31 ✅ ✅
source-postgres-strict-encrypt 2.0.31 🔵
(ignored)		 🔵
(ignored)
source-redshift 0.3.16 ✅ ✅
source-relational-db 0.3.1 🔵
(ignored)		 🔵
(ignored)
source-scaffold-java-jdbc 0.1.0 🔵
(ignored)		 🔵
(ignored)
source-sftp 0.1.2 ✅ ✅
source-snowflake 0.1.34 ✅ ✅
source-teradata 0.1.0 ✅ ✅
source-tidb 0.2.4 ✅ ✅
  • See "Actionable Items" below for how to resolve warnings and errors.

❌ Destinations (50)
Connector Version Changelog Publish
destination-azure-blob-storage 0.2.0 ✅ ✅
destination-bigquery 1.4.4 ✅ ✅
destination-bigquery-denormalized 1.4.1 ✅ ✅
destination-cassandra 0.1.4 ✅ ✅
destination-clickhouse 0.2.3 ✅ ✅
destination-clickhouse-strict-encrypt 0.2.3 🔵
(ignored)		 🔵
(ignored)
destination-csv 1.0.0 ✅ ✅
destination-databricks 1.0.2 ✅ ✅
destination-dev-null 0.3.0 🔵
(ignored)		 🔵
(ignored)
destination-doris 0.1.0 ✅ ✅
destination-dynamodb 0.1.7 ✅ ✅
destination-e2e-test 0.3.0 ✅ ✅
destination-elasticsearch 0.1.6 ✅ ✅
destination-elasticsearch-strict-encrypt 0.1.6 🔵
(ignored)		 🔵
(ignored)
destination-exasol 0.1.1 ✅ ✅
destination-gcs 0.3.0 ✅ ✅
destination-iceberg 0.1.0 ✅ ✅
destination-kafka 0.1.10 ✅ ✅
destination-keen 0.2.4 ✅ ✅
destination-kinesis 0.1.5 ✅ ✅
destination-local-json 0.2.11 ✅ ✅
destination-mariadb-columnstore 0.1.7 ✅ ✅
destination-mongodb 0.1.9 ✅ ✅
destination-mongodb-strict-encrypt 0.1.9 🔵
(ignored)		 🔵
(ignored)
destination-mqtt 0.1.3 ✅ ✅
destination-mssql 0.1.23 ✅ ✅
destination-mssql-strict-encrypt 0.1.23 🔵
(ignored)		 🔵
(ignored)
destination-mysql 0.1.20 ✅ ✅
destination-mysql-strict-encrypt ❌ 0.1.21
(mismatch: 0.1.20)		 🔵
(ignored)		 🔵
(ignored)
destination-oracle 0.1.19 ✅ ✅
destination-oracle-strict-encrypt 0.1.19 🔵
(ignored)		 🔵
(ignored)
destination-postgres 0.3.27 ✅ ✅
destination-postgres-strict-encrypt 0.3.27 🔵
(ignored)		 🔵
(ignored)
destination-pubsub 0.2.0 ✅ ✅
destination-pulsar 0.1.3 ✅ ✅
destination-r2 0.1.0 ✅ ✅
destination-redis 0.1.4 ✅ ✅
destination-redpanda 0.1.0 ✅ ✅
destination-redshift 0.4.8 ✅ ✅
destination-rockset 0.1.4 ✅ ✅
destination-s3 0.4.1 ✅ ✅
destination-s3-glue 0.1.7 ✅ ✅
destination-scylla 0.1.3 ✅ ✅
destination-selectdb 0.1.0 ✅ ✅
destination-snowflake 1.0.4 ✅ ✅
destination-starburst-galaxy 0.0.1 ✅ ✅
destination-teradata 0.1.1 ✅ ✅
destination-tidb 0.1.1 ✅ ❌
(diff seed version)
destination-vertica 0.1.0 ✅ ✅
destination-yugabytedb 0.1.1 ✅ ✅
  • See "Actionable Items" below for how to resolve warnings and errors.

✅ Other Modules (0)

Actionable Items

(click to expand)
Category Status Actionable Item
Version ❌
mismatch		 The version of the connector is different from its normal variant. Please bump the version of the connector.
⚠
doc not found		 The connector does not seem to have a documentation file. This can be normal (e.g. basic connector like source-jdbc is not published or documented). Please double-check to make sure that it is not a bug.
Changelog ⚠
doc not found		 The connector does not seem to have a documentation file. This can be normal (e.g. basic connector like source-jdbc is not published or documented). Please double-check to make sure that it is not a bug.
❌
changelog missing		 There is no chnagelog for the current version of the connector. If you are the author of the current version, please add a changelog.
Publish ⚠
not in seed		 The connector is not in the cloud or oss registry, so its publication status cannot be checked. This can be normal (e.g. some connectors are cloud-specific, and only listed in the cloud seed file). Please double-check to make sure that you have added a metadata.yaml file and the expected registries are enabled.

@cgardens cgardens changed the title grab bag of controversial clean up tasks grab bag of non-controversial clean up tasks May 27, 2023
cgardens
cgardens commented May 27, 2023
View reviewed changes
...s/base-java/src/main/java/io/airbyte/integrations/destination_async/AsyncStreamConsumer.java
// constructors right now.
try {

if (!isValidRecord.apply(message.getRecord().getData())) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we will have to figure out what to do when we move Redshift over to use this. But for that's the only one that uses it and trying to save this interface that relies on the data being deserialized isn't worth the effort now.

cgardens
cgardens commented May 27, 2023
View reviewed changes
...se-java-s3/src/main/java/io/airbyte/integrations/destination/s3/csv/CsvSerializedBuffer.java
@@ -62,12 +66,20 @@ protected void writeRecord(final AirbyteRecordMessage record) throws IOException

@Override
protected void flushWriter() throws IOException {
csvPrinter.flush();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in a single thread it wasn't really possible (or likely) for this to get called without accept getting called first. in a multi-threaded world that can actually happen pretty frequently, so when it log and move on.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

maybe leave as a comment?

cgardens
cgardens commented May 27, 2023
View reviewed changes
...n/java/io/airbyte/integrations/destination/buffered_stream_consumer/RecordSizeEstimator.java
@@ -71,7 +71,7 @@ public long getEstimatedByteSize(final AirbyteRecordMessage record) {
}

@VisibleForTesting
static long getStringByteSize(final JsonNode data) {
public static long getStringByteSize(final JsonNode data) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we end up using this in some tests later on

davinchia
davinchia approved these changes May 29, 2023
View reviewed changes
Copy link
Contributor

@davinchia davinchia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🏃🏻‍♂️

ryankfu
ryankfu approved these changes May 30, 2023
View reviewed changes
Copy link
Contributor

@ryankfu ryankfu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, is there a reason you want to merge into serialize messge consumer instead of master?

Base automatically changed from cgardens/serialized-message-consumer to master May 31, 2023 15:14
@cgardens cgardens force-pushed the cgardens/async-snowflake-boring branch from 8016643 to 6dc7de7 Compare May 31, 2023 15:25
@cgardens cgardens merged commit 480a43b into master May 31, 2023
@cgardens cgardens deleted the cgardens/async-snowflake-boring branch May 31, 2023 15:53
marcosmarxm pushed a commit to natalia-miinto/airbyte that referenced this pull request Jun 8, 2023
@cgardens @marcosmarxm
grab bag of non-controversial clean up tasks (airbytehq#26702)
dd9fd93
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ryankfu ryankfu ryankfu approved these changes

@davinchia davinchia davinchia approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cgardens @davinchia @ryankfu