-
Notifications
You must be signed in to change notification settings - Fork 7
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
Dataset project Id Feature implemented #242
Dataset project Id Feature implemented #242
Conversation
15dd6f6
to
bb07af0
Compare
Please add some description for context. Or attach a JIRA to this PR. Need more context on intended behavior projectId vs datasetProjectId in order to review this. |
String project = conf.getProject(); | ||
|
||
if (conf.getProject() == null) { | ||
return; |
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.
We should ideally throw a proper error like illegal argument exception in such case.
And all such checks should be inside the method getProject()
to ensure the method is null safe.
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.
Updated and removed
} | ||
String project = conf.getDatasetProject(); | ||
if (project == null) { | ||
return; |
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.
again similar : adding this check throwing exception within getDatasetProject()
would be better.
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.
Updated
@@ -147,22 +159,23 @@ public void initialize(DeltaTargetContext context) throws Exception { | |||
@Override | |||
public EventConsumer createConsumer(DeltaTargetContext context) throws IOException { | |||
Credentials credentials = conf.getCredentials(); | |||
String project = conf.getProject(); | |||
if (conf.getProject() == null) { | |||
throw new RuntimeException("Project id is not Present"); |
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.
similar : move this inside the getProject()
, will reduce code repetition.
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.
Updated and Removed
} | ||
String project = conf.getDatasetProject(); | ||
if (project == null) { | ||
throw new RuntimeException("Project Dataset id is not Present"); |
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.
similar : move this inside the getDatasetProject()
, will reduce code repetition.
Also it's better to throw IllegalArgumentException
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.
Updated and removed
@@ -352,5 +395,22 @@ private Credentials getCredentials() throws IOException { | |||
.createScoped(Collections.singleton("https://www.googleapis.com/auth/cloud-platform")); | |||
} | |||
} | |||
public String getDatasetProject() { | |||
// if it's "auto-detect" that means we need to detect the default project settings | |||
// for sandbox you can use `gcloud config set project my-project-id" to set it |
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.
Please remove or move the development
or debugging
related comments to readme.md
Like here, details about sandbox co-relates to the whole project , not just this method.
It's usually better to comment only what code is doing.
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.
Updated and Added in the .md file
@@ -267,7 +268,7 @@ public void testCreateConsumerRetryableFailureForGcsGet() throws Exception { | |||
} | |||
|
|||
@Test | |||
public void testCreateConsumerNonRetryableFailure() throws Exception { | |||
public void testCreateConsumerNonRetryableFailure() throws Exception { |
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.
nit : undo indentation
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.
Updated
bb07af0
to
0fb8884
Compare
docs/bigquery-cdcTarget.md
Outdated
account to run the job. If a temporary bucket needs to be created, the bucket will also be created in this project and | ||
'GCE Storage Bucket Admin' role on this project must be granted to the specified service account to create buckets. | ||
|
||
**Dataset Project ID**: Project the dataset belongs to. This is only required if the dataset is not |
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.
Please add "destination dataset" to be more clear
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.
Updated
widgets/bigquery-cdcTarget.json
Outdated
"label": "DataSet Project ID", | ||
"widget-type": "textbox", | ||
"widget-attributes": { | ||
"placeholder": "Project the dataset belongs to, if different from the project ID." |
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.
similar : Please add "destination dataset" to be more clear
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.
Updated
I tested the changes in my local, |
I am currently working on fix. |
Design Document:
https://docs.google.com/document/d/1KGM_5uexV5aKqdskPfJo5HzHJ7B8zTBXsHB1OFqxpZY/edit#heading=h.v1bttlen2xl8