-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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 write discover fetch event API #21506
Conversation
Airbyte Code Coverage
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xiaohansong I forgot to submit this review yesterday, sorry about that! Left a few comments and suggestions, let me know what you think of them. I can do another pass when needed, just let me know
airbyte-server/src/main/java/io/airbyte/server/handlers/helpers/CatalogConverter.java
Outdated
Show resolved
Hide resolved
@@ -181,6 +181,30 @@ public static io.airbyte.protocol.models.ConfiguredAirbyteCatalog toProtocolKeep | |||
return toProtocol(clone); | |||
} | |||
|
|||
public static io.airbyte.protocol.models.AirbyteCatalog toAirbyteCatalogProtocol( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the naming of this method is a little confusing compared to the method below it, toProtocol
. Perhaps we should rename the method below this to toProtocolConfigured
and then this method can be called toProtocol
?
At the very least, I would add some javadoc to this method and maybe try to include some guidance on when this method should be used versus the below method which returns a configured catalog
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah agreed. Mike was mentioning we should work with move team to consolidate these two states, but that was a bit out of scope for this PR/tickets.
@pmossman Ready for you to take another pass! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me, I think @mfsiega-airbyte should take a quick pass as well to make sure I didn't miss anything as this isn't my most familiar area of code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
What
#21279
To add a new internal API endpoint that workers will call.
How
Implement a new Api and hook it up with the existing configRepository logic. Have to implement a converter helper to translate from API model to protocol model.
Recommended reading order
x.java
y.python
🚨 User Impact 🚨
This function will not be used at this moment because workers are still calling DB directly. Thus no user impact.