Skip to content

Fix datastore to bigquery#90

Closed
vbambuch wants to merge 1 commit intoGoogleCloudPlatform:masterfrom
vbambuch:master
Closed

Fix datastore to bigquery#90
vbambuch wants to merge 1 commit intoGoogleCloudPlatform:masterfrom
vbambuch:master

Conversation

@vbambuch
Copy link
Copy Markdown

@vbambuch vbambuch commented Mar 19, 2020

  • add JSONPath attribute
  • use withSchema in WriteBigQuery job part

@googlebot
Copy link
Copy Markdown

googlebot commented Mar 19, 2020

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added the cla: no The PR submitter does not have a CLA label Mar 19, 2020
@vbambuch
Copy link
Copy Markdown
Author

vbambuch commented Mar 19, 2020

@googlebot I signed it!

@googlebot
Copy link
Copy Markdown

googlebot commented Mar 19, 2020

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@googlebot
Copy link
Copy Markdown

googlebot commented Mar 19, 2020

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes The PR submitter has a CLA and removed cla: no The PR submitter does not have a CLA labels Mar 19, 2020
@stale
Copy link
Copy Markdown

stale bot commented May 27, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the obsolete The PR hasn't had activity in 45 days label May 27, 2020
@vbambuch
Copy link
Copy Markdown
Author

vbambuch commented May 28, 2020

Okey...and what should I do? Do I need to make any other changes before you review it? 🤔

@sabhyankar sabhyankar self-requested a review May 28, 2020 17:36
@stale stale bot removed the obsolete The PR hasn't had activity in 45 days label May 28, 2020
@sabhyankar
Copy link
Copy Markdown
Member

sabhyankar commented May 28, 2020

Thanks for the PR @vbambuch - Ill take a look at this shortly

@vbambuch
Copy link
Copy Markdown
Author

vbambuch commented May 28, 2020

Great! Thank you @sabhyankar.

sabhyankar
sabhyankar requested changes May 29, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@sabhyankar sabhyankar left a comment

Choose a reason for hiding this comment

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

Hi @vbambuch - I have left some feedback on the PR. Note that we would want the change to be fully backward compatible. i.e The pipeline should work even when the user chooses to run this without the new --jsonPath flag.

Were you able to execute the template, with and without the flag? In the latter case, you would be writing to an existing table.

src/main/java/com/google/cloud/teleport/templates/DatastoreToBigQuery.java
void setOutputTableSpec(ValueProvider<String> value);

@Validation.Required
@Description("JSON file with BigQuery Schema description")
Copy link
Copy Markdown
Member

@sabhyankar sabhyankar May 29, 2020

Choose a reason for hiding this comment

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

Looking through the path, this is the actual stringified json schema and not exactly the path to the schema.

src/main/java/com/google/cloud/teleport/templates/DatastoreToBigQuery.java

void setOutputTableSpec(ValueProvider<String> value);

@Validation.Required
Copy link
Copy Markdown
Member

@sabhyankar sabhyankar May 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

IMO, this will not have any effect on a parameter wrapped inside the ValueProvider. We should remove this and the other places this annotation is used.

src/main/java/com/google/cloud/teleport/templates/DatastoreToBigQuery.java
.withoutValidation()
.withSchema(
ValueProvider.NestedValueProvider.of(
options.getJSONPath(),
Copy link
Copy Markdown
Member

@sabhyankar sabhyankar May 29, 2020

Choose a reason for hiding this comment

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

What happens when the user doesnt provide this json? Note the @Required annotation will not be useful here.

src/main/java/com/google/cloud/teleport/templates/DatastoreToBigQuery.java
"WriteBigQuery",
BigQueryIO.writeTableRows()
.withoutValidation()
.withSchema(
Copy link
Copy Markdown
Member

@sabhyankar sabhyankar May 29, 2020

Choose a reason for hiding this comment

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

Instead why not use withJsonSchema and having to do all that parsing.

src/main/java/com/google/cloud/teleport/templates/DatastoreToBigQuery.java
Comment on lines +66 to +69
private static final String BIGQUERY_SCHEMA = "BigQuery Schema";
private static final String NAME = "name";
private static final String TYPE = "type";
private static final String MODE = "mode";
Copy link
Copy Markdown
Member

@sabhyankar sabhyankar May 29, 2020

Choose a reason for hiding this comment

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

This would not be needed if you were to use withJsonSchema

@stale
Copy link
Copy Markdown

stale bot commented Jul 13, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the obsolete The PR hasn't had activity in 45 days label Jul 13, 2020
@stale stale bot closed this Jul 20, 2020
shreyakhajanchi pushed a commit that referenced this pull request Feb 11, 2025
@taherkl @pawankashyapollion
Cassandra Reverse Replication - Integration Tests (#2155)
1e284fe 
* Add IT for Cassandra (#86)

* Add IT for Cassandra Reverse Replication

* Added IT Fixes

* Added IT fixes

* removed logger

* removed unwanted system log

* Handle Catch

* Handle with safehandler

* Running load test

* revert unwanted commit

* Added IT FIXES

* Added UT and removed unwanted SOUT

* Added One to Many Datatype Transformation for IT (#90)

Added One to Many Datatype Transformation for IT
Fix PR review comments

* Handle IT for MYSQL

* Added ByteBuffer TO BigInteger

* Cassandra rr custom transfornation it test (#96)

* PR Review Comments (#97)

* Splotless fixes

* Handle Retry Missing Exception Category

* Rebase Issue fixes

---------

Co-authored-by: pawankashyapollion <v-pawan.kumar@ollion.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sabhyankar sabhyankar sabhyankar requested changes

Assignees

No one assigned

Labels

cla: yes The PR submitter has a CLA obsolete The PR hasn't had activity in 45 days

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@vbambuch @googlebot @sabhyankar