Skip to content

[BEAM-3433] Allow a GCP project to be explicitly set for a load job#5178

Merged
jkff merged 3 commits intoapache:masterfrom
kvncp:bug/BEAM-3433
May 17, 2018
Merged

[BEAM-3433] Allow a GCP project to be explicitly set for a load job#5178
jkff merged 3 commits intoapache:masterfrom
kvncp:bug/BEAM-3433

Conversation

@kvncp
Copy link

@kvncp kvncp commented Apr 19, 2018

I've added the needed string parameter to the BigQueryIO.write() function, and passed it through to the underlying class. Wanted to get some feedback before trying to write a test.

  1. Should I also add a ValueProvider interface?
  2. I've modified the constructor for WriteTables, which is public. Should I instead add a setter for that function or overload the constructor?
  3. Should I validate the this parameter is not set unless the Method is FILE_LOADS? It isn't harmful to set it otherwise, it is just ignored. Not sure what the recommendation is in that case.

@kvncp
Copy link
Author

kvncp commented Apr 19, 2018

Hi @chamikaramj , can you please take a look?

@chamikaramj chamikaramj requested review from chamikaramj and jkff and removed request for jkff April 27, 2018 18:43
@chamikaramj
Copy link
Contributor

Thanks. Will take a look.

cc: @jkff

* the write method is set to {@link Method#FILE_LOADS}. If omitted, the project of the
* destination table is used.
*/
public Write<T> withLoadJobProjectId(String loadJobProjectId) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a ValueProvider version too?

Copy link
Author

Choose a reason for hiding this comment

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

Done

@chamikaramj chamikaramj removed their request for review April 30, 2018 05:35
@jkff
Copy link
Contributor

jkff commented May 8, 2018

Hey @kvncp , any updates on this one?

@chamikaramj
Copy link
Contributor

@kvncp, please address comments to get this into next release.

@kvncp
Copy link
Author

kvncp commented May 16, 2018

Sorry, was on vacation recently and just catching up here. Addressing comments now!

@kvncp
Copy link
Author

kvncp commented May 16, 2018

@jfkk, @chamikaramj, I've addressed comments, and tests are passing.

@chamikaramj
Copy link
Contributor

LGTM.

@jkff jkff merged commit 8cf0eeb into apache:master May 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants