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

gisripa · 2024-07-27T01:40:06Z

What

Deferring the deletes until end of the sync to only delete on receving a COMPLETE for stream status.

How

Marking all the existing objects which matches the given pathFormat. This logic is similar to the earlier cleanUpObjects, instead of deleting them immediately, collecting all the object keys marked for delete and sending them to onClose function for actual deletion.

Review guide

New methods in BlobStorageOperations interface to support querying and cleaning the objects in a deferred fashion.
Storing keys marked for deletion in WriteConfig. Note that it is not thread-safe but onStart and onClose are only called once so nothing else is mutating that list in between.
Logical change in S3ConsumerFactory for deferring the deletion action
Test verifying that existing objects are left intact on failed attempts in OVERWRITE mode

User Impact

Can this PR be safely reverted and rolled back?

YES 💚
NO ❌

vercel · 2024-07-27T01:40:07Z

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

Name	Status	Preview	Comments	Updated (UTC)
airbyte-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Aug 14, 2024 8:40pm

gisripa · 2024-07-27T01:40:26Z

Destination S3: Deferred deletes on sync success in OVERWRITE #42579 👈
S3 Dest emits V2 fields and captures failures #42409 : 1 other dependent PR (#42542 )
S3 Destination Uses Async Framework #42405 : 2 other dependent PRs (#42543 , #42924 )
master

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

Join @gisripa and the rest of your teammates on Graphite

edgao

lgtm, had some fyi/questions

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

...destinations/src/main/kotlin/io/airbyte/cdk/integrations/destination/s3/S3ConsumerFactory.kt

...stinations/src/main/kotlin/io/airbyte/cdk/integrations/destination/s3/S3StorageOperations.kt

...estFixtures/kotlin/io/airbyte/cdk/integrations/destination/s3/S3DestinationAcceptanceTest.kt

...stinations/src/main/kotlin/io/airbyte/cdk/integrations/destination/s3/S3StorageOperations.kt

edgao

one last question, lgtm once resolved

edgao · 2024-08-13T15:09:58Z

...stinations/src/main/kotlin/io/airbyte/cdk/integrations/destination/s3/S3StorageOperations.kt

+        objectPath: String,
+        maxKeysPerPage: Int = 1000,
+        pageConsumer: (List<S3ObjectSummary>) -> Unit
+    ) {


are you intentionally keeping the regex matching stuff outside this method? (i.e. the Pattern.compile(getRegexFormat(namespace, streamName, pathFormat)) + objectSumaries.filter { regex.matcher(it).matches() } thing)

yeah. wasn't sure if all usages are doing filter at the time of refactoring (turns out they seem to be)

johnny-schmidt · 2024-08-13T16:12:34Z

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

+     * will be filtered with generationId metadata strictly less than [currentGenerationId]
+     * @return List of keys of the objects
+     */
+    abstract fun listExistingObjects(


Not requesting a change to this, but I wonder if in the new CDK we should try to keep the BL out of the storage operations as much as possible.

Like this is really an object search with a metadata comparator?

johnny-schmidt · 2024-08-13T16:16:36Z

...destinations/src/main/kotlin/io/airbyte/cdk/integrations/destination/s3/S3ConsumerFactory.kt

-                        stream,
-                        outputBucketPath,
-                        pathFormat
+                    writeConfig.objectsFromOldGeneration.addAll(


Another raised CDK question: what's the best way to handle accumulated stream state. Is appending to the config the right way? Does it make things less manageable?

this is only remnant in S3 land, we avoided attaching to WriteConfig (or rather killed that object all together) and saving states at instance level of that StreamOperation class. If there is a "state" machine then the FSM executor should accumulate and pass it along right ?

...stinations/src/main/kotlin/io/airbyte/cdk/integrations/destination/s3/S3StorageOperations.kt

gisripa · 2024-08-14T18:48:07Z

/publish-java-cdk

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

gisripa · 2024-08-14T19:27:00Z

Success sync with pre-release

octavia-squidington-iii added the CDK Connector Development Kit label Jul 27, 2024

This was referenced Jul 27, 2024

S3 Destination Uses Async Framework #42405

Merged

S3 Dest emits V2 fields and captures failures #42409

Merged

time types now logical numerics w/o strings #42542

Merged

Destination S3: Connector reacting to generationId and minGenId #42543

Merged

gisripa force-pushed the epic-8349-certify-s3/issue-8556-s3-v2-fields-gt branch from 2c71cb7 to d66a66b Compare July 27, 2024 01:41

gisripa force-pushed the gireesh/07-26-s3-no-downtime-full-refresh branch 2 times, most recently from 005b367 to f2bdd27 Compare July 29, 2024 05:17

gisripa force-pushed the epic-8349-certify-s3/issue-8556-s3-v2-fields-gt branch from d66a66b to 2b602b5 Compare July 29, 2024 05:18

gisripa force-pushed the gireesh/07-26-s3-no-downtime-full-refresh branch from f2bdd27 to d1c1c0a Compare July 29, 2024 05:18

johnny-schmidt force-pushed the epic-8349-certify-s3/issue-8556-s3-v2-fields-gt branch from 2b602b5 to cc558fa Compare July 29, 2024 18:02

gisripa force-pushed the epic-8349-certify-s3/issue-8556-s3-v2-fields-gt branch from cdf2021 to 32f91da Compare July 29, 2024 18:27

gisripa force-pushed the gireesh/07-26-s3-no-downtime-full-refresh branch from d1c1c0a to 3b439ea Compare July 29, 2024 18:27

gisripa force-pushed the epic-8349-certify-s3/issue-8556-s3-v2-fields-gt branch from 32f91da to 196731e Compare July 30, 2024 01:14

gisripa force-pushed the gireesh/07-26-s3-no-downtime-full-refresh branch 2 times, most recently from 14faf69 to 96cbf8f Compare July 30, 2024 01:30

gisripa changed the title ~~s3-no-downtime-full-refresh~~ Destination S3: Deferred deletes on sync success in OVERWRITE Jul 30, 2024

gisripa marked this pull request as ready for review July 30, 2024 15:20

gisripa requested a review from a team as a July 30, 2024 15:20

edgao approved these changes Jul 30, 2024

View reviewed changes

gisripa force-pushed the epic-8349-certify-s3/issue-8556-s3-v2-fields-gt branch from 196731e to 1627fdb Compare July 30, 2024 18:40

gisripa requested a review from a team as a code owner July 30, 2024 18:40

gisripa force-pushed the gireesh/07-26-s3-no-downtime-full-refresh branch from 96cbf8f to 4ac9531 Compare July 30, 2024 18:40

gisripa force-pushed the epic-8349-certify-s3/issue-8556-s3-v2-fields-gt branch from e6055c8 to 475d5c2 Compare July 31, 2024 00:14

gisripa force-pushed the gireesh/07-26-s3-no-downtime-full-refresh branch from 4ac9531 to 1aaf8f9 Compare July 31, 2024 00:14

johnny-schmidt force-pushed the epic-8349-certify-s3/issue-8556-s3-v2-fields-gt branch 2 times, most recently from 123df63 to 6629bba Compare July 31, 2024 16:46

johnny-schmidt reviewed Jul 31, 2024

View reviewed changes

...stinations/src/main/kotlin/io/airbyte/cdk/integrations/destination/s3/S3StorageOperations.kt Outdated Show resolved Hide resolved

gisripa force-pushed the gireesh/07-26-s3-no-downtime-full-refresh branch from 9b0f98a to 94bb9fe Compare August 12, 2024 23:58

edgao approved these changes Aug 13, 2024

View reviewed changes

octavia-squidington-iv requested a review from a team August 13, 2024 15:12

johnny-schmidt force-pushed the epic-8349-certify-s3/issue-8556-s3-v2-fields-gt branch from 51c23f8 to 1844494 Compare August 13, 2024 15:34

johnny-schmidt reviewed Aug 13, 2024

View reviewed changes

gisripa force-pushed the epic-8349-certify-s3/issue-8556-s3-v2-fields-gt branch from 1844494 to 33e3e0d Compare August 13, 2024 21:26

gisripa force-pushed the gireesh/07-26-s3-no-downtime-full-refresh branch from 94bb9fe to 67436cc Compare August 13, 2024 21:27

johnny-schmidt force-pushed the epic-8349-certify-s3/issue-8556-s3-v2-fields-gt branch from 33e3e0d to 1844494 Compare August 13, 2024 21:41

johnny-schmidt reviewed Aug 13, 2024

View reviewed changes

...stinations/src/main/kotlin/io/airbyte/cdk/integrations/destination/s3/S3StorageOperations.kt Outdated Show resolved Hide resolved

johnny-schmidt force-pushed the epic-8349-certify-s3/issue-8556-s3-v2-fields-gt branch from 1844494 to 194365d Compare August 13, 2024 22:16

johnny-schmidt approved these changes Aug 13, 2024

View reviewed changes

johnny-schmidt force-pushed the epic-8349-certify-s3/issue-8556-s3-v2-fields-gt branch 2 times, most recently from 491ad7c to a626bae Compare August 14, 2024 15:54

Base automatically changed from epic-8349-certify-s3/issue-8556-s3-v2-fields-gt to master August 14, 2024 16:41

gisripa force-pushed the gireesh/07-26-s3-no-downtime-full-refresh branch 2 times, most recently from 733226f to 8918b13 Compare August 14, 2024 17:47

gisripa removed the request for review from a team August 14, 2024 17:54

gisripa force-pushed the gireesh/07-26-s3-no-downtime-full-refresh branch 2 times, most recently from 152d411 to d3b7688 Compare August 14, 2024 18:35

gisripa force-pushed the gireesh/07-26-s3-no-downtime-full-refresh branch from d3b7688 to 1f789b2 Compare August 14, 2024 18:57

s3-no-downtime-full-refresh

adfe484

gisripa force-pushed the gireesh/07-26-s3-no-downtime-full-refresh branch from 1f789b2 to adfe484 Compare August 14, 2024 20:34

octavia-squidington-iii added the area/documentation Improvements or additions to documentation label Aug 14, 2024

vercel bot deployed to Preview August 14, 2024 20:40 View deployment

gisripa merged commit cba3ccd into master Aug 14, 2024
35 checks passed

gisripa deleted the gireesh/07-26-s3-no-downtime-full-refresh branch August 14, 2024 21:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

gisripa commented Jul 27, 2024 •

edited

Loading

vercel bot commented Jul 27, 2024 •

edited

Loading

gisripa commented Jul 27, 2024 •

edited

Loading

edgao left a comment

edgao left a comment

edgao Aug 13, 2024

gisripa Aug 13, 2024

johnny-schmidt Aug 13, 2024

johnny-schmidt Aug 13, 2024

gisripa Aug 13, 2024

gisripa commented Aug 14, 2024 •

edited by github-actions bot

Loading

gisripa commented Aug 14, 2024

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

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

Conversation

gisripa commented Jul 27, 2024 • edited Loading

What

How

Review guide

User Impact

Can this PR be safely reverted and rolled back?

vercel bot commented Jul 27, 2024 • edited Loading

gisripa commented Jul 27, 2024 • edited Loading

edgao left a comment

Choose a reason for hiding this comment

edgao left a comment

Choose a reason for hiding this comment

edgao Aug 13, 2024

Choose a reason for hiding this comment

gisripa Aug 13, 2024

Choose a reason for hiding this comment

johnny-schmidt Aug 13, 2024

Choose a reason for hiding this comment

johnny-schmidt Aug 13, 2024

Choose a reason for hiding this comment

gisripa Aug 13, 2024

Choose a reason for hiding this comment

gisripa commented Aug 14, 2024 • edited by github-actions bot Loading

gisripa commented Aug 14, 2024

gisripa commented Jul 27, 2024 •

edited

Loading

vercel bot commented Jul 27, 2024 •

edited

Loading

gisripa commented Jul 27, 2024 •

edited

Loading

gisripa commented Aug 14, 2024 •

edited by github-actions bot

Loading