Add PartialSerialisedMessage test. #27452

davinchia · 2023-06-17T04:47:23Z

What

Describe what the change is solving
It helps to add screenshots if it affects the frontend.

How

Describe the solution

🚨 User Impact 🚨

Are there any breaking changes? What is the end result perceived by the user?

For connector PRs, use this section to explain which type of semantic versioning bump occurs as a result of the changes. Refer to our Semantic Versioning for Connectors guidelines for more information. Breaking changes to connectors must be documented by an Airbyte engineer (PR author, or reviewer for community PRs) by using the Breaking Change Release Playbook.

If there are breaking changes, please merge this PR with the 🚨🚨 emoji so changelog authors can further highlight this if needed.

Pre-merge Actions

Expand the relevant checklist and delete the others.

New Connector

Community member or Airbyter

Community member? Grant edit access to maintainers (instructions)
Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
Connector version is set to 0.0.1
- Dockerfile has version 0.0.1
Documentation updated
- Connector's README.md
- Connector's bootstrap.md. See description and examples
- docs/integrations/<source or destination>/<name>.md including changelog with an entry for the initial version. See changelog example
- docs/integrations/README.md

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
If new credentials are required for use in CI, add them to GSM. Instructions.

Updating a connector

Community member or Airbyter

Grant edit access to maintainers (instructions)
Unit & integration tests added

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
If new credentials are required for use in CI, add them to GSM. Instructions.

Connector Generator

Issue acceptance criteria met
PR name follows PR naming conventions
If adding a new generator, add it to the list of scaffold modules being tested
The generator test modules (all connectors with -scaffold in their name) have been updated with the latest scaffold by running ./gradlew :airbyte-integrations:connector-templates:generator:testScaffoldTemplates then checking in your changes
Documentation which references the generator is updated as needed

github-actions · 2023-06-17T04:52:41Z

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`	✅	❌ (diff seed version)
`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.33`	✅	✅
`source-postgres-strict-encrypt`	`2.0.33`	🔵 (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`	✅	❌ (diff seed version)
`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.4`	✅	❌ (diff seed version)
`destination-clickhouse-strict-encrypt`	`0.2.4`	🔵 (ignored)	🔵 (ignored)
`destination-csv`	`1.0.0`	✅	✅
`destination-databricks`	`1.1.0`	✅	✅
`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.24`	✅	✅
`destination-mssql-strict-encrypt`	`0.1.24`	🔵 (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.5`	✅	✅
`destination-starburst-galaxy`	`0.0.1`	✅	✅
`destination-teradata`	`0.1.1`	✅	✅
`destination-tidb`	`0.1.3`	✅	✅
`destination-vertica`	`0.1.0`	✅	✅
`destination-yugabytedb`	`0.1.1`	✅	✅

See "Actionable Items" below for how to resolve warnings and errors.

👀 Other Modules (1)

base-normalization

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.

davinchia · 2023-06-17T06:03:46Z

...s/base-java/src/main/java/io/airbyte/integrations/destination_async/AsyncStreamConsumer.java

@@ -124,15 +123,17 @@ public void accept(final String messageString, final Integer sizeInBytes) throws
   * @param messageString the string to deserialize
   * @return PartialAirbyteMessage if the message is valid, empty otherwise
   */
-  private Optional<PartialAirbyteMessage> deserializeAirbyteMessage(final String messageString) {
+  @VisibleForTesting
+  public static Optional<PartialAirbyteMessage> deserializeAirbyteMessage(final String messageString) {


we should add tests for this since this is now a critical piece

...s/base-java/src/main/java/io/airbyte/integrations/destination_async/AsyncStreamConsumer.java

davinchia · 2023-06-17T06:17:02Z

...se-java/src/test/java/io/airbyte/integrations/destination_async/AsyncStreamConsumerTest.java

@@ -258,25 +259,20 @@ private static void consumeRecords(final AsyncStreamConsumer consumer, final Col

  // NOTE: Generates records at chunks of 160 bytes
  @SuppressWarnings("SameParameterValue")
-  private static List<PartialAirbyteMessage> generateRecords(final long targetSizeInBytes) {


this was previously doing too much.

simplified to only generate AirbyteMessages and split out the logic to generate expected records into the verifyRecords function

ryankfu · 2023-06-19T19:11:08Z

...s/base-java/src/main/java/io/airbyte/integrations/destination_async/AsyncStreamConsumer.java

            return partial.withSerialized(messageString);
+          } else {


@davinchia added another case where I believe we can have bad data which is when messageString is neither a STATE or RECORD message. In this case, this should not be passed to the consumer. I haven't personally seen this happen but this now more closely matches the description that PartialAirbyteMessage should only contain either STATE or RECORD. Lmk if you think this is unnecessary. Not sure if the platform filters out messages

We cannot guarantee messages arrive at the Destination ungarbled, so we definitely need to consider this case.

Less decided whether we should fail fast or log an error.. I think we should do the simplest for now and throw a RTE to make sure we don't drop data.

I'm wondering if this happens in practice somehow. We should add logging + test this out before committing this code.

Sounds good, I'll add an RTE here, add some logging, publish a new pre-release version, and run it overnight with some resets to see if this ever gets triggered but agree we should fail fast here

… message

ryankfu · 2023-06-20T18:08:02Z

Going to merge this into ryan/fix-record-data branch, clean up any changes, rebase and request a review from Destination's team to also go over what has changed, what was fixed and the level of testing needed to confirm things are back to green

* Test deserialise. * Add tests. * Simplify and fix tests. * Format. * Adds tests for deserializeAirbyteMessage * Adds tests for deserializeAirbyteMessage with bad data * Cleans up deserializeAirbyteMessage and throws Exception when invalid message * More code cleanup --------- Co-authored-by: ryankfu <ryan.fu@airbyte.io>

* Adds data as JsonNode to pass through, running into memory issues so add JVM args to attach VisualVM * JsonNode * Lowers the optimal batch size to see if this improvements movement * Fixes NPE by checking if PartialAirbyteMessage contains a PartialAirbyteRecord * Fixes config switch when config is not explicitly set (config migration needed) * Adds logic to check if queue has elements before getting timeOfLastMessage * Add PartialSerialisedMessage test. (#27452) * Test deserialise. * Add tests. * Simplify and fix tests. * Format. * Adds tests for deserializeAirbyteMessage * Adds tests for deserializeAirbyteMessage with bad data * Cleans up deserializeAirbyteMessage and throws Exception when invalid message * More code cleanup --------- Co-authored-by: ryankfu <ryan.fu@airbyte.io> * 🤖 Auto format destination-snowflake code [skip ci] * Cleans up code w/o JVM args & rebase * 🤖 Auto format destination-snowflake code [skip ci] * Adds breadcrumb on the STATE message deviation and where the deserialize/serialize is done to unpack * 🤖 Auto format destination-snowflake code [skip ci] * Adds back line formatter removed and comment describing rational for lower batchSize * Bumps Snowflake version and type checks * Added note to remove PartialAirbyteRecordMessage with low resource testing * Automated Commit - Format and Process Resources Changes * Fix issue with multiple namespaces in snowflake not writing to the correct staging schema * Fix issue with multiple namespaces in snowflake not writing to the correct staging schema * remove stage name maniuplating method * update readme * Source Stripe: update credit_notes expected records (#27941) * Source Zendesk Talk: update expected records (#27942) * Source Xero: update expected records (#27943) * Metadata: Persist Registry entries (#27766) * DNC * Update poetry * Update dagster * Apply partition * Get metadata entry * Use helpers * Write registry entry to appropriate location * Delete when registry removed * Update to use new file (broken) * Render registry from registry entries * Run format * Fix plural issue * Update to all metadata file blobs * Fix test * Update to all blobs * Add ignore validation error for version logic * Rename to max_run_request * Pedros review * Ella suggestions Co-authored-by: Ella Rohm-Ensing <erohmensing@gmail.com> * Update airbyte-ci/connectors/metadata_service/orchestrator/orchestrator/assets/registry_entry.py Co-authored-by: Ella Rohm-Ensing <erohmensing@gmail.com> * Update naming * Add tests for connector type and deletion * Test safe parse * Format --------- Co-authored-by: Octavia Squidington III <octavia-squidington-iii@sers.noreply.github.com> Co-authored-by: Ella Rohm-Ensing <erohmensing@gmail.com> * Fix Dagster Deploy Failure (#27955) * Add pydantic * Add pydantic to orchestration deploy pipeline * 🐛 Source Jira: update expected records (#27951) * Source Jira: update expected records * Update issues expected records * Source Zendesk Chat: update expected records (#27965) * 🐛 Source Pipedrive: update expected records (#27967) * 🐛 Source Pinterest: update expected records (#27964) * ✨ Source Amazon-Ads: Add streams for portfolios and sponsored brands v3 (#27607) * Add stream for sponsored brands v3 * Add new stream Portfolios * Source Google Search Console: added discover and googleNews to searchType (#27952) * added discover and googleNews to searchType * updated changelog * fixed types for streams * 🎉 Source Instagram: Improve, refactor `STATE` management (#27908) * add test for enabling * update versions * fix test * update other snowflake loading method types * remove standard --------- Co-authored-by: ryankfu <ryan.fu@airbyte.io> Co-authored-by: Davin Chia <davinchia@gmail.com> Co-authored-by: octavia-squidington-iii <octavia-squidington-iii@users.noreply.github.com> Co-authored-by: ryankfu <ryankfu@users.noreply.github.com> Co-authored-by: Augustin <augustin@airbyte.io> Co-authored-by: Arsen Losenko <20901439+arsenlosenko@users.noreply.github.com> Co-authored-by: Ben Church <ben@airbyte.io> Co-authored-by: Octavia Squidington III <octavia-squidington-iii@sers.noreply.github.com> Co-authored-by: Ella Rohm-Ensing <erohmensing@gmail.com> Co-authored-by: Anatolii Yatsuk <35109939+tolik0@users.noreply.github.com> Co-authored-by: Daryna Ishchenko <80129833+darynaishchenko@users.noreply.github.com> Co-authored-by: Baz <oleksandr.bazarnov@globallogic.com>

davinchia added 2 commits June 16, 2023 19:42

Test deserialise.

2811d3c

Add tests.

4537256

davinchia changed the title ~~Davinchia/add test~~ Add PartialSerialisedMessage test. Jun 17, 2023

davinchia added 2 commits June 16, 2023 23:01

Simplify and fix tests.

4a0c4ae

Format.

2151007

davinchia commented Jun 17, 2023

View reviewed changes

...s/base-java/src/main/java/io/airbyte/integrations/destination_async/AsyncStreamConsumer.java Outdated Show resolved Hide resolved

davinchia commented Jun 17, 2023

View reviewed changes

Adds tests for deserializeAirbyteMessage

b96f2b2

octavia-squidington-iii added area/connectors Connector related issues connectors/destination/snowflake labels Jun 17, 2023

Adds tests for deserializeAirbyteMessage with bad data

9c7a22f

ryankfu reviewed Jun 19, 2023

View reviewed changes

ryankfu added 2 commits June 19, 2023 21:48

Cleans up deserializeAirbyteMessage and throws Exception when invalid…

19b7f68

… message

More code cleanup

51db7ac

ryankfu marked this pull request as ready for review June 20, 2023 18:08

ryankfu requested review from a team as code owners June 20, 2023 18:08

ryankfu merged this pull request into ryan/fix-record-data Jun 20, 2023
21 of 23 checks passed

ryankfu deleted the davinchia/add-test branch June 20, 2023 18:08

ryankfu mentioned this pull request Jun 21, 2023

🐛 Destination Snowflake: Async adds data to the PartialAirbyteRecordMessage to pass through #27342

Closed

jbfbell mentioned this pull request Jul 3, 2023

🐛 Destination Snowflake: Merge old snowflake work #27935

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add PartialSerialisedMessage test. #27452

Add PartialSerialisedMessage test. #27452

davinchia commented Jun 17, 2023

github-actions bot commented Jun 17, 2023 •

edited

Loading

❌ Sources (33)

❌ Destinations (50)

👀 Other Modules (1)

Actionable Items

davinchia Jun 17, 2023

davinchia Jun 17, 2023

ryankfu Jun 19, 2023

davinchia Jun 19, 2023

ryankfu Jun 20, 2023

ryankfu commented Jun 20, 2023

Add PartialSerialisedMessage test. #27452

Add PartialSerialisedMessage test. #27452

Conversation

davinchia commented Jun 17, 2023

What

How

Recommended reading order

🚨 User Impact 🚨

Pre-merge Actions

Community member or Airbyter

Airbyter

Community member or Airbyter

Airbyter

github-actions bot commented Jun 17, 2023 • edited Loading

Affected Connector Report

❌ Sources (33)

❌ Destinations (50)

👀 Other Modules (1)

Actionable Items

davinchia Jun 17, 2023

Choose a reason for hiding this comment

davinchia Jun 17, 2023

Choose a reason for hiding this comment

ryankfu Jun 19, 2023

Choose a reason for hiding this comment

davinchia Jun 19, 2023

Choose a reason for hiding this comment

ryankfu Jun 20, 2023

Choose a reason for hiding this comment

ryankfu commented Jun 20, 2023

github-actions bot commented Jun 17, 2023 •

edited

Loading