Async Code V0 #26324

davinchia · 2023-05-19T23:49:28Z

What

Split out the smallest set of reasonable changes from #26086 .

My goal was to split out the interface, as well as show how the interface it's meant to be used.

Follow up PRs:

Split out classes from BufferManager and add more tests there.
Add in the AsyncConsumer with tests.
Add in the StagingConsumer factory.

How

Split out the classes and add javadocs.

From original branch:

renamed StreamDestinationFlusher to DestinationFlushFunction.
renamed UploadWorkers to FlushWorkers.
also split out the MemoryManager class to its own class.

Did not touch any tests.

Some formatting changes.

🚨 User Impact 🚨

No user impact since all of this is in a new module.

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

…ager

* Set up initial supervisor and worker thread scaffolding. Set up WorkerConfig to move flush over. * Checkpoint: before moving staging operation interface. * Uber merge with Charles.

…ination

…byte into cgardens/async-destination

…ination

* memory manager * bring back close * use constant

…byte into cgardens/async-destination

- fix reclaiming memory from queues - suggest which knobs to turn next

davinchia · 2023-05-19T23:56:54Z

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

+import org.apache.commons.io.FileUtils;
+
+@Slf4j
+public class BufferManager {


this class can be further split and tested. will do so in a follow up PR

davinchia · 2023-05-20T00:04:48Z

PTAL @ryankfu @cgardens, this is the first set of PRs to get our code from #26086 into master.

With this first PR, I want to get the smallest reasonable set into master, with a focus on documenting the interfaces.

I was hoping we could do this more piecemeal, however there is really no way since all these classes are somewhat related.

Charles, I know you were hoping to comment and rewrite the queue bits. Feel free to do so here or in a follow up PR. My preference is a follow up PR to keep things small. The queue bits are well tested so I don't believe having this in master is 'that bad'.

davinchia · 2023-05-20T00:05:25Z

...ns/bases/base-java/src/main/java/io/airbyte/integrations/destination_async/FlushWorkers.java

I am happy to take naming suggestions.

Maybe AsyncDataUploader? As suggested by ChatGPT 😛

Decent idea. My thought was to keep this in line with the interface name of DestinationFlushFunction. Let's iterate on this next week.

davinchia · 2023-05-20T00:05:35Z

...e-java/src/main/java/io/airbyte/integrations/destination_async/DestinationFlushFunction.java

I am happy to take naming suggestions.

ryankfu

I'm happy with this V0 merge, since it doesn't include any of the StagingConsumerFactory changes it doesn't affect snowflake directly. Agree that comments and additional breaking out the classes can be in follow up PR

It does feel a little weird that there's Postgres/MySQL changes in here but those look like spotlessJava linting

davinchia · 2023-05-20T20:42:14Z

I am merging this in as it doesn't affect any actual connectors.

Follow up to #26324 - here we split up the BufferManager and add tests and comments. - Split up the buffer manager class into -> BufferManager, BufferEnqueue and BufferDequeue. - Move all buffer related code to the buffers package. - Rename test classes to match this split. - Add java docs and tests as part of this split. - Simplify the BufferDequeue interface to return a set streams representing the buffered streams instead of the underlying map of buffers. This lets us keep the memory queue package private. - all getYMethods now return Optionals for better error handling. This would have resulted in NPEs previously.

Follow up after #26324 . Introduce the AsyncStreamConsumer. After this, one more PR to add the Staging Consumer changes in.

Split out the smallest set of reasonable changes from #26086 . My goal was to split out the interface, as well as show how the interface it's meant to be used. Follow up PRs: - Split out classes from BufferManager and add more tests there. - Add in the AsyncConsumer with tests. - Add in the StagingConsumer factory.

Follow up to #26324 - here we split up the BufferManager and add tests and comments. - Split up the buffer manager class into -> BufferManager, BufferEnqueue and BufferDequeue. - Move all buffer related code to the buffers package. - Rename test classes to match this split. - Add java docs and tests as part of this split. - Simplify the BufferDequeue interface to return a set streams representing the buffered streams instead of the underlying map of buffers. This lets us keep the memory queue package private. - all getYMethods now return Optionals for better error handling. This would have resulted in NPEs previously.

Follow up after #26324 . Introduce the AsyncStreamConsumer. After this, one more PR to add the Staging Consumer changes in.

Split out the smallest set of reasonable changes from airbytehq#26086 . My goal was to split out the interface, as well as show how the interface it's meant to be used. Follow up PRs: - Split out classes from BufferManager and add more tests there. - Add in the AsyncConsumer with tests. - Add in the StagingConsumer factory.

Follow up to airbytehq#26324 - here we split up the BufferManager and add tests and comments. - Split up the buffer manager class into -> BufferManager, BufferEnqueue and BufferDequeue. - Move all buffer related code to the buffers package. - Rename test classes to match this split. - Add java docs and tests as part of this split. - Simplify the BufferDequeue interface to return a set streams representing the buffered streams instead of the underlying map of buffers. This lets us keep the memory queue package private. - all getYMethods now return Optionals for better error handling. This would have resulted in NPEs previously.

Follow up after airbytehq#26324 . Introduce the AsyncStreamConsumer. After this, one more PR to add the Staging Consumer changes in.

cgardens and others added 30 commits May 15, 2023 13:07

scaffold

b732bfd

tidy up

7c73e06

implement memory aware queue

c4c71fe

crib what we need from buffereed stream consumer, excluding state man…

1800d98

…ager

Upload workers (#26102)

2cbd9e7

* Set up initial supervisor and worker thread scaffolding. Set up WorkerConfig to move flush over. * Checkpoint: before moving staging operation interface. * Uber merge with Charles.

Checkpoint: before merging master.

3788758

Merge remote-tracking branch 'origin/master' into cgardens/async-dest…

2a34e38

…ination

Implement one full pass.

287cc70

Add logic to close.

b6b8ce6

Merge remote-tracking branch 'origin' into cgardens/async-destination

a287892

Merge branch 'cgardens/async-destination' of github.com:airbytehq/air…

83819b2

…byte into cgardens/async-destination

Checkpoint: working end-to-end test.

63f9012

Checkpoint: start pulling classes out.

e94088b

Merge remote-tracking branch 'origin' into cgardens/async-destination

178151b

Merge branch 'cgardens/async-destination' of github.com:airbytehq/air…

f190870

…byte into cgardens/async-destination

Merge remote-tracking branch 'origin/master' into cgardens/async-dest…

c9f02b0

…ination

New destination flusher interface.

5992484

Automated Change

4ccc3f1

add take in batch to dequeue (#26167)

719c4ac

Add queue usage loop.

7f5eda3

Automated Change

5591482

hook up memory manager to free (#26171)

9f95e44

* memory manager * bring back close * use constant

Merge remote-tracking branch 'origin' into cgardens/async-destination

1b6e8d5

Adds catalog for bottleneck_streams1 dataset

ca595a0

Merge branch 'cgardens/async-destination' of github.com:airbytehq/air…

0a90f83

…byte into cgardens/async-destination

Add debug loop.

e1bc45b

Fix unstash.

16422ff

Print thread pool info.

97c1180

Automated Change

b7f37c4

async destination fix reclaim (#26174)

0429ec8

- fix reclaiming memory from queues - suggest which knobs to turn next

davinchia added 2 commits May 19, 2023 16:54

Remove staging consumer factory test changes.

d3fe52e

Remove performance catalog changes.

baf91ea

davinchia commented May 19, 2023

View reviewed changes

davinchia marked this pull request as ready for review May 19, 2023 23:57

davinchia requested review from a team as code owners May 19, 2023 23:57

davinchia requested review from cgardens and ryankfu and removed request for a team May 20, 2023 00:02

davinchia commented May 20, 2023

View reviewed changes

Format and update.

e34f16d

ryankfu approved these changes May 20, 2023

View reviewed changes

Automated Change

84f5dc8

cgardens approved these changes May 20, 2023

View reviewed changes

davinchia merged commit 8bfbef2 into master May 20, 2023
6 of 7 checks passed

davinchia deleted the davinchia/async-code-0 branch May 20, 2023 20:41

davinchia mentioned this pull request May 21, 2023

Async Destination V0 - Split up BufferManager #26331

Merged

davinchia mentioned this pull request May 22, 2023

Async Destination V0: Async Stream Consumer #26366

Merged

davinchia added a commit that referenced this pull request May 22, 2023

Async Destination V0: Async Stream Consumer (#26366)

a7442e3

Follow up after #26324 . Introduce the AsyncStreamConsumer. After this, one more PR to add the Staging Consumer changes in.

nguyenaiden pushed a commit that referenced this pull request May 25, 2023

Async Destination V0: Async Stream Consumer (#26366)

17d9958

Follow up after #26324 . Introduce the AsyncStreamConsumer. After this, one more PR to add the Staging Consumer changes in.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Async Code V0 #26324

Async Code V0 #26324

davinchia commented May 19, 2023 •

edited

davinchia May 19, 2023

davinchia commented May 20, 2023

davinchia May 20, 2023

ryankfu May 20, 2023

davinchia May 20, 2023

davinchia May 20, 2023

ryankfu left a comment

davinchia commented May 20, 2023

Async Code V0 #26324

Async Code V0 #26324

Conversation

davinchia commented May 19, 2023 • edited

What

How

Recommended reading order

🚨 User Impact 🚨

Pre-merge Actions

Community member or Airbyter

Airbyter

Community member or Airbyter

Airbyter

davinchia May 19, 2023

Choose a reason for hiding this comment

davinchia commented May 20, 2023

davinchia May 20, 2023

Choose a reason for hiding this comment

ryankfu May 20, 2023

Choose a reason for hiding this comment

davinchia May 20, 2023

Choose a reason for hiding this comment

davinchia May 20, 2023

Choose a reason for hiding this comment

ryankfu left a comment

Choose a reason for hiding this comment

davinchia commented May 20, 2023

davinchia commented May 19, 2023 •

edited