Skip to content

PLUGIN-557: BigQuerySink fix for upsert operation in existing table but in different project #555

Merged
CuriousVini merged 1 commit intodata-integrations:developfrom
AdaptiveScale:bugfix/PLUGIN-557
Feb 26, 2021
Merged

PLUGIN-557: BigQuerySink fix for upsert operation in existing table but in different project #555
CuriousVini merged 1 commit intodata-integrations:developfrom
AdaptiveScale:bugfix/PLUGIN-557

Conversation

@flakrimjusufi
Copy link
Copy Markdown
Contributor

BigQuery sink pipeline fails doing upsert operation to existing table in different project. The table is partitioned by ingestion-time (day). Partition field is left empty.

JIRA: https://cdap.atlassian.net/browse/PLUGIN-557

@ardian4
Copy link
Copy Markdown
Contributor

ardian4 commented Feb 18, 2021

lgtm

private void configureTable(Schema schema) {
AbstractBigQuerySinkConfig config = getConfig();
Table table = BigQueryUtil.getBigQueryTable(config.getProject(), config.getDataset(),
Table table = BigQueryUtil.getBigQueryTable(config.getDatasetProject(), config.getDataset(),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

what if config.getProject() is provided? In that case we will rely on config.getDatasetProject()?

Copy link
Copy Markdown
Contributor Author

@flakrimjusufi flakrimjusufi Feb 22, 2021

Choose a reason for hiding this comment

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

If config.getProject() and config.getDatasetProject() are provided, then datasetProject will get priority.

If datasetProject is not defined, then config.getProject() will be set up in configuration.
That's how it is defined in AbstractBigQuerySinkConfig

@Nullable
public String getDatasetProject() {
if (GCPConfig.AUTO_DETECT.equalsIgnoreCase(datasetProject)) {
return ServiceOptions.getDefaultProjectId();
}
return Strings.isNullOrEmpty(datasetProject) ? getProject() : datasetProject;
}

Copy link
Copy Markdown
Member

@CuriousVini CuriousVini left a comment

Choose a reason for hiding this comment

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

Is there any unit test or integration test that can be added for this bug?

@flakrimjusufi
Copy link
Copy Markdown
Contributor Author

Is there any unit test or integration test that can be added for this bug?

We created this JIRA Ticket https://cdap.atlassian.net/browse/PLUGIN-606 in order to add integration test in the future because currently it would be hard to add without having another project for integration test.

@CuriousVini
Copy link
Copy Markdown
Member

/gcbrun

@CuriousVini
Copy link
Copy Markdown
Member

Has this fix been tested with dataset id being a macro?

@flakrimjusufi
Copy link
Copy Markdown
Contributor Author

Is there any unit test or integration test that can be added for this bug?

Has this fix been tested with dataset id being a macro?

Yes, this fix has been tested with dataset id being a macro.
There's no exception or anything like that.
If datasetId is a macro, the validation will not proceed.

This is the block of code that prevents any validation exception if any of the fields contains macro:

if (!config.shouldConnect()) {
return;
}

And this is the code of shouldConnect():

boolean shouldConnect() {
return !containsMacro(BigQuerySinkConfig.NAME_DATASET) &&
!containsMacro(BigQuerySinkConfig.NAME_TABLE) &&
!containsMacro(NAME_SERVICE_ACCOUNT_TYPE) &&
!(containsMacro(NAME_SERVICE_ACCOUNT_FILE_PATH) || containsMacro(NAME_SERVICE_ACCOUNT_JSON)) &&
!containsMacro(BigQuerySinkConfig.NAME_PROJECT) &&
!containsMacro(BigQuerySinkConfig.DATASET_PROJECT_ID) &&
!containsMacro(BigQuerySinkConfig.NAME_SCHEMA);
}

@CuriousVini CuriousVini added the build Trigger unit test build label Feb 26, 2021
@CuriousVini
Copy link
Copy Markdown
Member

/gcbrun

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

build Trigger unit test build

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants