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.

Sign up for GitHub

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

Jump to bottom

Destinations CDK: protocol message interop #45407

Merged
merged 2 commits into from
Sep 13, 2024
Merged

Destinations CDK: protocol message interop #45407

merged 2 commits into from
Sep 13, 2024

Conversation

edgao
Copy link
Contributor

@edgao edgao commented Sep 11, 2024 •
edited
Loading

fill out all the structs that mirror the protocol messages, and give them all functions to convert to the corresponding protocol object. This (a) gives us all the stuff we need to actually build destinations for real, and (b) gives tests an easier way to build all the protocol objects.

@vercel Vercel
Copy link

vercel bot commented Sep 11, 2024 •
edited
Loading

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 13, 2024 6:54pm

This was referenced Sep 11, 2024
Merged
Merged
Merged
@edgao Graphite App
Copy link
Contributor Author

edgao commented Sep 11, 2024 •
edited
Loading

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

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

@octavia-squidington-iii octavia-squidington-iii added the CDK Connector Development Kit label Sep 11, 2024
@edgao edgao force-pushed the edgao/message_renaming branch from 5470a36 to 6062525 Compare September 12, 2024 15:44
@edgao
Copy link
Contributor Author

edgao commented Sep 12, 2024

@johnny-schmidt this should be in better shape now - I also went in and implemented asProtocolMessage on the state messages.

(I'll fill out the rest of DestinationStream/DestinationCatalog + give them asProtocolObject methods later today. Should be purely mechanical.)

@edgao edgao force-pushed the edgao/message_renaming branch from 2730e88 to bc15bac Compare September 12, 2024 23:41
@edgao edgao changed the base branch from edgao/cdk_integration_tests to master September 12, 2024 23:41
@edgao edgao force-pushed the edgao/message_renaming branch 2 times, most recently from cfd61fd to 2b66f37 Compare September 13, 2024 16:16
edgao
edgao commented Sep 13, 2024
View reviewed changes
airbyte-cdk/bulk/core/load/src/main/kotlin/io/airbyte/cdk/message/DestinationMessage.kt Outdated
override val destinationStats: Stats? = null
) : CheckpointMessage() {
override fun withDestinationStats(stats: Stats) =
val streamCheckpoint: CheckpointMessage.StreamCheckpoint,
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 don't really understand how this was working previously. Explicitly use the child class here to avoid a recursive constructor

(I can't think of a better name to avoid this clash 🤷 )

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah that must have happened during the renaming. Weirdly seems to resolve correctly though?

Either Change the StreamCheckpoint/GlobalCheckpoint child classes to StreamCheckpointMessage and GlobalCheckpointMessage or change the StreamCheckpoint inner class to Checkpoint?

@edgao edgao force-pushed the edgao/message_renaming branch 3 times, most recently from db8daa0 to cb8dc89 Compare September 13, 2024 16:43
@edgao edgao marked this pull request as ready for review September 13, 2024 16:47
@edgao edgao requested a review from a team as a code owner September 13, 2024 16:47
johnny-schmidt
johnny-schmidt approved these changes Sep 13, 2024
View reviewed changes
Copy link
Contributor

@johnny-schmidt johnny-schmidt left a comment

Choose a reason for hiding this comment

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

Looks great! Also allows me to cross deserializer tests off the cdk todo list.

@edgao edgao force-pushed the edgao/message_renaming branch from cb8dc89 to 47c4481 Compare September 13, 2024 18:34
@edgao edgao enabled auto-merge (squash) September 13, 2024 18:34
@edgao edgao force-pushed the edgao/message_renaming branch from 47c4481 to 7ab8734 Compare September 13, 2024 18:54
@edgao edgao merged commit 58d241d into master Sep 13, 2024
26 checks passed
@edgao edgao deleted the edgao/message_renaming branch September 13, 2024 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johnny-schmidt johnny-schmidt johnny-schmidt approved these changes

Assignees
No one assigned
Labels
CDK Connector Development Kit
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@edgao @johnny-schmidt @octavia-squidington-iii