Rodi/refactor output consumer - #62862
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
👋 Greetings, Contributor!Here are some helpful tips and reminders for your convenience. Helpful Resources
PR Slash CommandsAirbyte Maintainers (that's you!) can execute the following slash commands on your PR:
|
|
|
|
|
|
|
Matt Bayley (mwbayley)
left a comment
Matt Bayley (mwbayley)
left a comment
There was a problem hiding this comment.
A few questions. Maybe the class hierarchy can be simpler?
| } | ||
|
|
||
| /** A simple [OutputConsumer] such as standard output or buffering test output consumer. */ | ||
| abstract class StandardOutputConsumer(clock: Clock) : BaseStdoutOutputConsumer(clock) |
There was a problem hiding this comment.
What's the point of this class? It extends the BaseStdoutOutputConsumer without adding any implementations.
| StandardOutputConsumer( | ||
| clock, | ||
| ) { | ||
| override fun withLockFlush() { |
There was a problem hiding this comment.
Where does the name withLockFlush come from? I don't see any locking.
| if (buffer.size() >= bufferByteSizeThresholdForFlush) { | ||
| withLockFlush() | ||
| } | ||
| } |
There was a problem hiding this comment.
We check buffer.size() both here and in the withLockFlush() function, which violates separation of concerns. Maybe move this size check into withLockFlush() and rename to something like maybeFlush()?
| } | ||
| } | ||
| } | ||
| abstract fun withLockFlush() |
There was a problem hiding this comment.
Can you help me understand why we need this abstract function? It feels internal to the implementations.
| core = 'extract' | ||
| toolkits = ['extract-jdbc', 'extract-cdc'] | ||
| cdk = '0.530' | ||
| cdk = 'local' |
There was a problem hiding this comment.
are we pinning to a CDK version, or no?
There was a problem hiding this comment.
I'm keeping local for now on RCs as the CDK is not calcified yet - frequent updates.
I'm going to set to a published version for the release.
…ames - 3.52.1: 2026-05-05 -> 2026-05-06 (PR #77787 merge date) - 3.50.5: 2025-07-30 -> 2025-07-31 (PR #63377 UTC merge date) - 3.50.3: 2025-07-18 -> 2025-07-22 (PR #63349 merge date) - 3.50.1-rc.1: 2025-07-08 -> 2025-07-10 (PR #62862 merge date) - Step 3 SSL modes: `require`/`verify-ca` -> `required`/`verify_ca` to match the actual mode values in the spec Co-Authored-By: bot_apk <apk@cognition.ai>
4 participants
Refactor SocketJsonOutputConsumer and StdoutOutputConsumer to share a single base class.