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

Unit tests for streams manager #45090

Merged

Conversation

johnny-schmidt
Copy link
Contributor

What

  • Unit tests for the streams manager
  • Removed all unnecessary/unused behavior
    • tracking bytes read per stream
  • Replaced java countdown latch with idiomatic kotlin "unit-channel-as-barrier"
  • Guards against irrational states
    • no counting after end-of-stream
    • no closing before end-of-stream
    • batch processing completeness requires end-of-stream
  • Test refactor: Moved commonly-used catalog mock to its own class (@Named)
  • YES 💚
  • NO ❌

@johnny-schmidt johnny-schmidt requested a review from a team as a code owner September 2, 2024 20:06
Copy link

vercel bot commented Sep 2, 2024

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 Sep 3, 2024 9:48pm

@octavia-squidington-iii octavia-squidington-iii added the CDK Connector Development Kit label Sep 2, 2024
@johnny-schmidt johnny-schmidt force-pushed the issue-9615/load-cdk-unit-tests-streams-manager branch from 322e28a to 8a98da0 Compare September 2, 2024 20:07
Base automatically changed from issue-9615/load-cdk-unit-tests-state-manager to master September 3, 2024 21:45
@johnny-schmidt johnny-schmidt force-pushed the issue-9615/load-cdk-unit-tests-streams-manager branch from 8a98da0 to d635c2c Compare September 3, 2024 21:48
@@ -33,68 +35,98 @@ class DefaultStreamsManager(
return streamManagers[stream] ?: throw IllegalArgumentException("Stream not found: $stream")
}

override suspend fun awaitAllStreamsComplete() {
override suspend fun awaitAllStreamsClosed() {
streamManagers.forEach { (_, manager) -> manager.awaitStreamClosed() }
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO: error handling

* Count the end-of-stream. Expect this exactly once. Expect no further `countRecordIn`, and
* expect that `markClosed` will always occur after this.
*/
fun countEndOfStream(): Long
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 a weird name. I'm not really sure what "counting the end of stream" means. Total number of records in the stream?

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand when reading further.
I think the notion of "counting" is off here. I don't expect count to increment a counter. We can leave it as is for now

streamManagers.forEach { (_, manager) -> manager.awaitStreamClosed() }
}
}

/** Manages the state of a single stream. */
interface StreamManager {
fun countRecordIn(sizeBytes: Long): Long
/** Count incoming record and return the record's *index*. */
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks to me like most of the functions here are setting state on the stream itself. Maybe this is just part of a Stream?

listOf(
Pair(stream1, SetRecordCount(10)),
Pair(stream1, AddPersisted(0, 9)),
Pair(stream1, ExpectPersistedUntil(9)),
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a bit confusing. Is the persistedUntil the message number or message count?

@johnny-schmidt johnny-schmidt merged commit af58faa into master Sep 3, 2024
31 checks passed
@johnny-schmidt johnny-schmidt deleted the issue-9615/load-cdk-unit-tests-streams-manager branch September 3, 2024 23:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CDK Connector Development Kit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants