-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Managed BigQueryIO #31486
base: master
Are you sure you want to change the base?
Managed BigQueryIO #31486
Conversation
R: @chamikaramj |
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control |
R: @chamikaramj |
…hods can instead be done in Dataflow service side
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 but this is a lot so good to get a second set of eyes. I could easily have missed something.
@@ -36,6 +36,9 @@ dependencies { | |||
permitUnusedDeclared project(":sdks:java:io:google-cloud-platform") // BEAM-11761 | |||
implementation project(":sdks:java:extensions:schemaio-expansion-service") | |||
permitUnusedDeclared project(":sdks:java:extensions:schemaio-expansion-service") // BEAM-11761 | |||
implementation project(":sdks:java:managed") | |||
permitUnusedDeclared project(":sdks:java:managed") // BEAM-11761 |
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.
Notes, no action required on this PR:
- This is a link to Jira, so probably there's a github issue it is migrated to
- This should be equivalent to
runtimeOnly
because it is "implementation" but no static references to it. I would guess this works the same, or else the uberjar plugin might not treat it right. - Putting these deps into a docker container without making an uber jar would honestly be better in the case where it does end up in a container, so we keep the original jar metadata.
...pache/beam/sdk/io/gcp/bigquery/providers/BigQueryStorageWriteApiSchemaTransformProvider.java
Outdated
Show resolved
Hide resolved
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.
Thanks.
.../java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryFileLoadsWriteSchemaTransformProvider.java
Show resolved
Hide resolved
.../java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryFileLoadsWriteSchemaTransformProvider.java
Show resolved
Hide resolved
.../java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryFileLoadsWriteSchemaTransformProvider.java
Outdated
Show resolved
Hide resolved
|
||
if (!Strings.isNullOrEmpty(configuration.getCreateDisposition())) { | ||
CreateDisposition createDisposition = | ||
CreateDisposition.valueOf(configuration.getCreateDisposition().toUpperCase()); |
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.
As a larger point, I think we should do any transform overriding in job submission (BQ modes for batch/streaming etc.) so that we can just upgrade in the backend (at least in the first version).
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.
Do you mean making this switch in the SDK (ie. construction time)? I assumed we had settled on making it a runner side decision
Some decisions are actually dependent on the runner (e.g. at least one streaming mode in Dataflow)
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.
Do you mean making this switch in the SDK (ie. construction time)? I assumed we had settled on making it a runner side decision
Yeah. Added some comments to the relavent doc.
No description provided.