Skip to content

Conversation

@itsankit-google
Copy link
Contributor

@itsankit-google itsankit-google commented Nov 3, 2022

Follow up PR of #1152.

It includes the support of creating common staging bucket for Dataplex and BQ pushdown plugins.

@itsankit-google itsankit-google added the build Trigger unit test build label Nov 3, 2022
@itsankit-google itsankit-google force-pushed the PLUGIN-1418 branch 3 times, most recently from 8bb475f to 889788a Compare November 3, 2022 17:40
// Get the bucket name for the specified location.
bucket = BigQueryUtil.getBucketNameForLocation(bucketPrefix, datasetLocation);
}
return bucket;
Copy link
Contributor

Choose a reason for hiding this comment

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

How does this behave is the supplied Bucket is nullable and there is no prefix set? As far as I can tell, the returned bucket will be null.

If this is the case, please add the @Nullable annotation to this function.

Following question is: Should this function return a bucket name if not supplied and no prefix is set?

Copy link
Contributor Author

@itsankit-google itsankit-google Nov 7, 2022

Choose a reason for hiding this comment

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

Yeah, it is expected to return null in that case. Added the @Nullable annotation.

No we don't expect this method to return a bucket name if not supplied and if no prefix is set.
BigQuerySinkUtils#configureBucket and BigQuerySourceUtils#getOrCreateBucket expect the bucket argument to be nullable and will keep the existing behaviour.

Copy link
Contributor

@fernst fernst left a comment

Choose a reason for hiding this comment

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

lgtm

@itsankit-google itsankit-google merged commit 724e092 into develop Nov 10, 2022
@itsankit-google itsankit-google deleted the PLUGIN-1418 branch November 10, 2022 13:17
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.

2 participants