-
Notifications
You must be signed in to change notification settings - Fork 4.4k
Support managed jdbc io (Postgres) #36034
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
Conversation
a1a365a
to
2af4afe
Compare
2af4afe
to
eb9366a
Compare
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment |
Will take a look later, but from a quick skim, it looks like we're missing translation logic in this PR |
Assigning reviewers: R: @liferoad for label python. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
Thanks for taking a look. I've been able to call both the managed and unmanaged versions of these transforms from Java and Python. Could you provide some pointers on where this translation logic is used? |
model/pipeline/src/main/proto/org/apache/beam/model/pipeline/v1/external_transforms.proto
Show resolved
Hide resolved
Added translation logic. However for my educational purpose, where is this translation called? |
when converting the transform to its proto representation: beam/sdks/java/core/src/main/java/org/apache/beam/sdk/util/construction/PTransformTranslation.java Line 499 in d28fd64
|
Then why could my previous integration tests and my manual call in python work without it? |
Because it can still build a viable transform proto just fine, and run in a normal pipeline. The extra translation logic is just needed to add more information that the Dataflow managed service can make use of to do upgrades. |
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.
Left a few comments, but looks good overall! Thanks @shunping
...main/java/org/apache/beam/sdk/io/jdbc/providers/ReadFromPostgresSchemaTransformProvider.java
Outdated
Show resolved
Hide resolved
.../main/java/org/apache/beam/sdk/io/jdbc/providers/WriteToPostgresSchemaTransformProvider.java
Outdated
Show resolved
Hide resolved
sdks/java/io/jdbc/src/test/java/org/apache/beam/sdk/io/jdbc/JdbcIOPostgresIT.java
Outdated
Show resolved
Hide resolved
Thanks for clarifying this. cc'ed @liferoad |
We should have the readme file to document all these requirements. |
Managed JDBCIO (Part 1) - Postgres
The first PR to make JDBCIO into managed IO.
Notice that we are still using the GCP IO expansion service jar. I will move it to the designated expansion service jar after finishing all the four supported databases.