Abacn commented on code in PR #31698:
URL: https://github.com/apache/beam/pull/31698#discussion_r1744397433
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryServicesImpl.java:
##########
@@ -251,14 +259,10 @@ Bigquery getClient() {
@Override
public void startLoadJob(JobReference jobRef, JobConfigurationLoad
loadConfig)
throws InterruptedException, IOException {
- Map<String, String> labelMap = new HashMap<>();
Job job =
new Job()
.setJobReference(jobRef)
- .setConfiguration(
- new JobConfiguration()
- .setLoad(loadConfig)
-
.setLabels(this.bqIOMetadata.addAdditionalJobLabels(labelMap)));
Review Comment:
addAdditionalJobLabels will add beam job id which becomes available at some
later stage, and now it is called much earlier. Not sure if this is a concern,
but can we minimize the scope of the change such that just replace labelMap to
this.jobLabels here?
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryServicesImpl.java:
##########
@@ -221,19 +221,27 @@ static class JobServiceImpl implements
BigQueryServices.JobService {
private final ApiErrorExtractor errorExtractor;
private final Bigquery client;
private final BigQueryIOMetadata bqIOMetadata;
+ private final Map<String, String> jobLabels;
@VisibleForTesting
JobServiceImpl(Bigquery client) {
+ this(client, new HashMap<>());
+ }
+
+ @VisibleForTesting
+ JobServiceImpl(Bigquery client, Map<String, String> jobLabels) {
this.errorExtractor = new ApiErrorExtractor();
this.client = client;
this.bqIOMetadata = BigQueryIOMetadata.create();
+ this.jobLabels = jobLabels;
Review Comment:
should we make a copy for the original map, so this.jobLabels gauranteed to
be a HashMap, to avoid mutate the original one (and in case the parameter
passed in is immutable)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscr...@beam.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
