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.

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

add configuration for cdc queue size #35739

Conversation

rodireich
Copy link
Contributor

What

Similar to other connectors we allow an admin to configure mssql source's queue size used to buffer cdc events to help in case of OOM.

How

Add queue size configuration used by debezium handler rather than hard coding the size to 10k

@rodireich rodireich linked an issue Mar 1, 2024 that may be closed by this pull request
Copy link

vercel bot commented Mar 1, 2024

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

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
airbyte-docs ⬜️ Ignored (Inspect) Visit Preview Mar 5, 2024 11:33pm

@octavia-squidington-iii octavia-squidington-iii added area/connectors Connector related issues connectors/source/mssql area/documentation Improvements or additions to documentation labels Mar 1, 2024
@rodireich rodireich marked this pull request as ready for review March 1, 2024 01:38
@rodireich rodireich requested a review from a team as a code owner March 1, 2024 01:38
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.*;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a formatting error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked as well.
the formatter actually is the one that put it in place.
I think we either reached some threshold

Copy link
Contributor

Choose a reason for hiding this comment

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

that's super annoying if the formatter adds files to your PR...

@@ -167,7 +163,7 @@ public static List<AutoCloseableIterator<AirbyteMessage>> getCdcReadIterators(fi
true,
firstRecordWaitTime,
subsequentRecordWaitTime,
AirbyteDebeziumHandler.QUEUE_CAPACITY,
queueSize,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'll just use this opportunity to ask questions:

From my understanding, this underlying queue is a BlockingQueue only used outside of debezium - in other words we do not change debezium properties like max.queue.size right?

And since this basically defines the size of the queue, it could mean -

  1. the larger the number is, we could probably benefit from having fewer bottleneck issues
  2. the lower the number is, the less preserved memory it could cost.

Is that right? In that case, I feel we could benefit from gathering data on the max queue size used in the sync

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right in what you're saying. The queue we control is our queue buffering events coming out of debezium.
The reason I suspect this is the queue that causes an OOM is that the size of max.queue.size is a constant of 8K and we're also setting the max.queue.size.in.bytes which sets another limit of size (the first max reached is the limit).

@stephane-airbyte
Copy link
Contributor

let's add a counter in the queue implementation and put some data in the logs, so that we don't have to keep guessing in the future. We should be able to easily gather number of elements in the queue and total size in the queue

@octavia-squidington-iii octavia-squidington-iii added the CDK Connector Development Kit label Mar 5, 2024
@stephane-airbyte
Copy link
Contributor

stephane-airbyte commented Mar 5, 2024

/publish-java-cdk

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

@stephane-airbyte stephane-airbyte force-pushed the 21498-source-mssql-cdc-job-is-getting-failed-due-to-oom-error branch from 5be001c to 2fa69df Compare March 5, 2024 21:43
Copy link
Contributor

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

Join @rodireich and the rest of your teammates on Graphite Graphite

@stephane-airbyte stephane-airbyte force-pushed the 21498-source-mssql-cdc-job-is-getting-failed-due-to-oom-error branch 2 times, most recently from dfc3860 to 74fda84 Compare March 5, 2024 23:20
@stephane-airbyte
Copy link
Contributor

stephane-airbyte commented Mar 5, 2024

/publish-java-cdk

🕑 https://github.com/airbytehq/airbyte/actions/runs/8164381486
❌ Publish Java CDK version=0.23.15 failed!

@stephane-airbyte stephane-airbyte force-pushed the 21498-source-mssql-cdc-job-is-getting-failed-due-to-oom-error branch from 74fda84 to a48a677 Compare March 5, 2024 23:33
@stephane-airbyte
Copy link
Contributor

stephane-airbyte commented Mar 5, 2024

/publish-java-cdk force=true

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

@@ -127,8 +127,7 @@ public final C shared(String imageName, List<? extends NamedContainerModifier<C>
@SuppressWarnings("unchecked")
@Deprecated
public final C exclusive(String imageName, String... methods) {
return exclusive(imageName,
(NamedContainerModifier<C>) Stream.of(methods).map(n -> new NamedContainerModifierImpl<C>(n, resolveModifierByName(n))).toList());
return exclusive(imageName, Stream.of(methods).map(n -> new NamedContainerModifierImpl<C>(n, resolveModifierByName(n))).toList());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks this was failing on warning

@stephane-airbyte
Copy link
Contributor

stephane-airbyte commented Mar 6, 2024

/approve-and-merge reason="source-mssql tests are still broken"

@airbytehq airbytehq deleted a comment from octavia-approvington Mar 6, 2024
@stephane-airbyte
Copy link
Contributor

/approve-and-merge reason="source-mssql tests are still broken"

@octavia-approvington
Copy link
Contributor

Lets do it!
giddddy-up

@octavia-approvington octavia-approvington merged commit f9e73cf into master Mar 6, 2024
27 of 29 checks passed
@octavia-approvington octavia-approvington deleted the 21498-source-mssql-cdc-job-is-getting-failed-due-to-oom-error branch March 6, 2024 00:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/connectors Connector related issues area/documentation Improvements or additions to documentation CDK Connector Development Kit connectors/source/mssql
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Source MSSQL: CDC job is getting failed due to OOM error
5 participants