Skip to content

Conversation

@pritukam
Copy link
Contributor

No description provided.

@googlebot googlebot added the cla: yes The PR submitter has a CLA label Nov 22, 2019
@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no The PR submitter does not have a CLA and removed cla: yes The PR submitter has a CLA labels Nov 26, 2019
@googlebot
Copy link

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 Nov 26, 2019
Copy link
Member

Choose a reason for hiding this comment

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

What is the unit? Also specify default: 6000

Copy link
Member

Choose a reason for hiding this comment

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

Do we want the default to be false? Probably better to set the default to true?

Copy link
Member

Choose a reason for hiding this comment

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

Same, do we want to ignore by default?

Copy link
Member

Choose a reason for hiding this comment

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

Set default to True?

Copy link
Member

Choose a reason for hiding this comment

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

Unused Logger.

Copy link
Member

Choose a reason for hiding this comment

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

Should we call this successful-json-conversion or something less ambiguous?

Copy link
Member

Choose a reason for hiding this comment

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

Same here.

Copy link
Member

Choose a reason for hiding this comment

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

Will this parsing always be successful? If not, we should put this in a try-catch block and use separate tags to catch these parsing failures (with metric-counters).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The function will always get a valid json. Valid json will always get parsed as Bson documents

Copy link
Member

Choose a reason for hiding this comment

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

Yes in the context of this pipeline. However, what happens when I attempt to use the public static class and pass a String (which is not a valid json)?

Copy link
Member

Choose a reason for hiding this comment

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

Missing javadoc.

Copy link
Member

Choose a reason for hiding this comment

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

Can you reformat/reorder the imports?

@pritukam pritukam force-pushed the master branch 2 times, most recently from f74503b to ab3d32a Compare December 11, 2019 14:49
Copy link
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.

LGTM

@sabhyankar sabhyankar added the Google LGTM Approval of a pull request to be merged into the repository label Dec 11, 2019
@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no The PR submitter does not have a CLA and removed cla: yes The PR submitter has a CLA labels Dec 11, 2019
@pritukam pritukam force-pushed the master branch 2 times, most recently from 1782bbf to ab3d32a Compare December 11, 2019 17:00
@googlebot
Copy link

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 Dec 11, 2019
ryanmcdowell pushed a commit that referenced this pull request Dec 11, 2019
PiperOrigin-RevId: 285039202
@ryanmcdowell ryanmcdowell merged commit 5a53b1d into GoogleCloudPlatform:master Dec 11, 2019
shreyakhajanchi pushed a commit that referenced this pull request Jan 23, 2025
* Dml integration (#53)

* Added extensive UT

Added extensive UT

* Cassandra pr bug fixes (#57)

* Cassandra Consolidate Unit Test case and Regression testing fixes (#58)

* Added Mapping fixes

* Added Spoltles fixes

* Added Consolidated fixes

* Added TODO

* Addess Data and Time

* Cassandra pr bug fixes (#64)

* Handle TypeHandler Parsing issue fixes (#65)

Co-authored-by: pawankashyapollion <v-pawan.kumar@ollion.com>

* Added Safe handle (#68)

* Handle LocalTime For Time Data Type In Cassandra (#69)

* Cassandra pr bug fixes (#70)

* Handle Timestamp Fixes (#72)

* Added Code Combined in a single way

* Address The Unwanted Hop

* Cassandra pr bug fixes (#75)

* Added PR Review Comments

* Remove NamesCol Dependecy as spannerTableName is same as In Given Mapping

* Added spannerTableId for fetching Mapping

* Removed SpannerToID and also Updated Session file with proper structure

* Timestamp in milisecond

* removed assertNotNull from UT wherever possible

* Added Fixes

* Added Note Instead of Question

* -- review fixes (#78)

* Added Bytes to hex to blob conversion

* Handling Bytes as Binary encoded As of now

* Passing Null Value to Primary Key as well for cassandra

* Added UT fixes

* Added UT refectoring

* Reverse merge confict fixes

---------

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

Labels

cla: yes The PR submitter has a CLA Google LGTM Approval of a pull request to be merged into the repository MongoDB size/XXL

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants