S3 Destination Uses Async Framework by johnny-schmidt · Pull Request #42405 · airbytehq/airbyte

Johnny Schmidt (johnny-schmidt) · 2024-07-22T16:25:18Z

What

This accomplishes

speed performance
checkpointing
counts

How

STTCPW: the old serial process

buffers files locally by stream in destination format
flushes when over threshold

Specifically, it used a single SerializedBufferingStrategy that handled creating file buffers by stream w/ format type and enforcing thresholds. State management, etc was handled upstream.

So I assigned one newly-created SerializedBufferingStrategy per async worker per flush.

Practically, this means that when a memory buffer reaches threshold, it starts, flushes, and closes a file buffer in a single pass. (This is all handled in S3DestinationFlushFunction)

Review guide

This is going to be the first in a series of breaking-changes for s3, so I'll be stacking pr's.

async consumer
support refreshes [+update metadata]

CHANGES WILL BE MERGEABLE WITH BREAKING CHANGES AFTER 2. 3 and 4 will constitute breaking changes.

Code changes consist of

test changes (added trace messages so consumer wouldn't fail)
automated name changes (S3Destination... -> S3DestinationAsync...)
new async consumer initialization (BaseS3Destination)
** NOTE: the flush function is initialized with a SerializedBufferingStragey provider so that no two threads use the same object!
stitching async to sync (S3DestinationFlushFunction)
** convert PartialAirbyteMessage to AirbyteMessage
** new SerializableBuffer method for flushing a single stream (so I can flush the buffers as soon as it finishes
** "default namespace": the async consumer replaces null namespaces w/ a default taken from the schema, which can break stuff in the s3 code that expects null; So I could either 1) make async accept null OR 2) thread an empty string through S3; I chose the latter to avoid impacting other code, though it is kind of ugly.

User Impact

Local testing shows a 2.5x speedup (from about 10mb/s peak to 25mb/s). However perf metrics collected from production data suggest we won't realize that speedup because few upstream sources perform over 10mb/s. So user impact might be minimal.

I'm not sure if counts or checkpoints are meaningful w/o metadata changes that will be part of the refresh PR? Practical effects of counts is that the platform crashes the sync if they don't line up, so we might see more reliability at the cost of higher failure rates.

BREAKING CHANGES

Technically this isn't a breaking change, but because the Async Consumer is dependent on trace messages to work properly:

metadata must be updated to indicate we support refresh
This only works with platform version > 0.63.7 in OSS, so they have to either upgrade the platform or downgrade the connector.

Gireesh Sreepathi (@gisripa) is working on this now.

Can this PR be safely reverted and rolled back?

[x ] YES 💚
NO ❌

I think? Keep me honest

vercel · 2024-07-22T16:25:22Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment

Name	Status	Preview	Comments	Updated (UTC)
airbyte-docs	⬜️ Ignored (Inspect)	Visit Preview		Aug 1, 2024 9:44pm

Johnny Schmidt (johnny-schmidt) · 2024-07-22T16:25:36Z

S3 Destination Uses Async Framework #42405 : 3 dependent PRs (#42409 , #42543 , #42924 ) 👈
master

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join Johnny Schmidt (@johnny-schmidt) and the rest of your teammates on Graphite

Edward Gao (edgao)

several small comments, lgtm once those are resolved. Like I said in one of my comments - either I'm missing something, or returning a new BufferingStrategy on every flush seems like something we'd want to do in all destinations?

Johnny Schmidt (johnny-schmidt) · 2024-07-23T00:31:34Z

Very misleading typo here, fixed locally. (states => stats)

Edward Gao (edgao)

one last lgtm for formality's sake

Gireesh Sreepathi (gisripa)

BaseGcsDestination.kt is failing compilation. Either we can choose to keep it backward-compat or split the changes of cdk and publish and work off of a published version.

Gireesh Sreepathi (gisripa) · 2024-08-01T21:18:28Z

/publish-java-cdk force=true

🕑 https://github.com/airbytehq/airbyte/actions/runs/10205922390
✅ Successfully published Java CDK version=0.44.0!

Stephane Geneix (stephane-airbyte) · 2024-08-01T21:37:22Z

let's simplify this.

Stephane Geneix (stephane-airbyte) · 2024-08-01T21:37:29Z

Stephane Geneix (stephane-airbyte) · 2024-08-01T21:37:38Z

Stephane Geneix (stephane-airbyte) · 2024-08-01T21:37:47Z

Stephane Geneix (stephane-airbyte) · 2024-08-01T21:38:43Z

nit: we can use string templates

Stephane Geneix (stephane-airbyte) · 2024-08-01T21:41:14Z

oh god this is ugly. All because generationId can be 0... 😠

Stephane Geneix (stephane-airbyte) · 2024-08-01T21:45:39Z

@@ -63,17 +60,19 @@ protected constructor(
        catalog: ConfiguredAirbyteCatalog,


[Re: line +58]

should we make the method final?

See this comment inline on Graphite.

Octavia Squidington III (octavia-squidington-iii) added area/connectors Connector related issues CDK Connector Development Kit connectors/destination/s3 labels Jul 22, 2024

Johnny Schmidt (johnny-schmidt) marked this pull request as ready for review July 22, 2024 16:40

Johnny Schmidt (johnny-schmidt) requested review from a team as code owners July 22, 2024 16:40

Johnny Schmidt (johnny-schmidt) mentioned this pull request Jul 22, 2024

S3 Dest emits V2 fields and captures failures #42409

Merged

1 of 2 tasks

Johnny Schmidt (johnny-schmidt) requested a review from Edward Gao (edgao) July 22, 2024 21:06

Edward Gao (edgao) approved these changes Jul 22, 2024

View reviewed changes

Johnny Schmidt (johnny-schmidt) force-pushed the epic-8349-certify-s3/issue-8554-s3-async-gt branch from 27652f2 to 470eee5 Compare July 23, 2024 00:19

Johnny Schmidt (johnny-schmidt) commented Jul 23, 2024

View reviewed changes

Johnny Schmidt (johnny-schmidt) force-pushed the epic-8349-certify-s3/issue-8554-s3-async-gt branch 2 times, most recently from f5414ea to 5c9ea03 Compare July 23, 2024 00:50

Edward Gao (edgao) approved these changes Jul 23, 2024

View reviewed changes

Gireesh Sreepathi (gisripa) force-pushed the epic-8349-certify-s3/issue-8554-s3-async-gt branch from 5c9ea03 to 4d4e5db Compare July 24, 2024 17:53

Johnny Schmidt (johnny-schmidt) force-pushed the epic-8349-certify-s3/issue-8554-s3-async-gt branch from 4d4e5db to b7615bb Compare July 25, 2024 17:16

Johnny Schmidt (johnny-schmidt) mentioned this pull request Jul 25, 2024

time types now logical numerics w/o strings #42542

Merged

1 of 2 tasks

Gireesh Sreepathi (gisripa) force-pushed the epic-8349-certify-s3/issue-8554-s3-async-gt branch from b7615bb to 4d4e5db Compare July 25, 2024 18:43

Gireesh Sreepathi (gisripa) mentioned this pull request Jul 25, 2024

Destination S3: Connector reacting to generationId and minGenId #42543

Merged

1 of 2 tasks

Gireesh Sreepathi (gisripa) force-pushed the epic-8349-certify-s3/issue-8554-s3-async-gt branch 2 times, most recently from 88a707c to 5ac38e1 Compare July 25, 2024 18:47

Gireesh Sreepathi (gisripa) reviewed Jul 25, 2024

View reviewed changes

Johnny Schmidt (johnny-schmidt) force-pushed the epic-8349-certify-s3/issue-8554-s3-async-gt branch from 5ac38e1 to 5c1dd5e Compare July 26, 2024 01:07

Gireesh Sreepathi (gisripa) force-pushed the epic-8349-certify-s3/issue-8554-s3-async-gt branch from d5e9a3b to 9dc273c Compare July 26, 2024 16:49

Johnny Schmidt (johnny-schmidt) force-pushed the epic-8349-certify-s3/issue-8554-s3-async-gt branch from 9dc273c to fe42bca Compare July 26, 2024 21:51

Octavia Squidington III (octavia-squidington-iii) added the area/documentation Improvements or additions to documentation label Jul 26, 2024

vercel Bot deployed to Preview July 26, 2024 22:42

View deployment

Gireesh Sreepathi (gisripa) mentioned this pull request Jul 27, 2024

Destination S3: Deferred deletes on sync success in OVERWRITE #42579

Merged

2 tasks

Johnny Schmidt (johnny-schmidt) and others added 14 commits August 1, 2024 14:12

S3 Destination Uses Async Framework

3e086bc

Addressed review comments

87ed364

Restore gcs cdk compilation

c5b021c

format fixes

febbd5d

Compile check failures

daedb72

metadata and formatting

e9cda59

s3-recovery-genid

ed19a07

s3-dats-genid

e615a93

[airbyte-cdk] add name property to http_client for convience (#42508)

938a88b

S3 Destination Uses Async Framework

d43d8fa

Addressed review comments

d71acb6

Compile check failures

bb95d4a

metadata and formatting

7398be4

s3-recovery-genid

89db9d6

Gireesh Sreepathi (gisripa) force-pushed the epic-8349-certify-s3/issue-8554-s3-async-gt branch from 3f06465 to b9aed56 Compare August 1, 2024 21:14

Stephane Geneix (stephane-airbyte) reviewed Aug 1, 2024

View reviewed changes

Comment thread ...inations/src/main/kotlin/io/airbyte/cdk/integrations/destination/s3/BlobStorageOperations.kt Outdated

Stephane Geneix (stephane-airbyte) reviewed Aug 1, 2024

View reviewed changes

s3-dats-genid

165a2e5

Gireesh Sreepathi (gisripa) force-pushed the epic-8349-certify-s3/issue-8554-s3-async-gt branch from b9aed56 to 165a2e5 Compare August 1, 2024 21:44

Stephane Geneix (stephane-airbyte) reviewed Aug 1, 2024

View reviewed changes

Gireesh Sreepathi (gisripa) merged commit 2320f78 into master Aug 1, 2024

Gireesh Sreepathi (gisripa) deleted the epic-8349-certify-s3/issue-8554-s3-async-gt branch August 1, 2024 22:06

Johnny Schmidt (johnny-schmidt) mentioned this pull request Aug 7, 2024

S3 1.0 Breaking Change Documentation #43374

Closed

Kévin (kev-datams) mentioned this pull request Aug 19, 2024

Destination GCS: Failed to convert json to avro #14064

Open

		@@ -63,17 +60,19 @@ protected constructor(
		catalog: ConfiguredAirbyteCatalog,

Conversation

Johnny Schmidt (johnny-schmidt) commented Jul 22, 2024 • edited by gisripa Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

How

Review guide

User Impact

BREAKING CHANGES

Can this PR be safely reverted and rolled back?

Uh oh!

vercel Bot commented Jul 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Johnny Schmidt (johnny-schmidt) commented Jul 22, 2024 • edited by gisripa Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Edward Gao (edgao) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Edward Gao (edgao) left a comment

Choose a reason for hiding this comment

Uh oh!

Gireesh Sreepathi (gisripa) left a comment

Choose a reason for hiding this comment

Uh oh!

Gireesh Sreepathi (gisripa) commented Aug 1, 2024 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Johnny Schmidt (johnny-schmidt) commented Jul 22, 2024 •

edited by gisripa

Loading

vercel Bot commented Jul 22, 2024 •

edited

Loading

Johnny Schmidt (johnny-schmidt) commented Jul 22, 2024 •

edited by gisripa

Loading

Gireesh Sreepathi (gisripa) commented Aug 1, 2024 •

edited by github-actions Bot

Loading