-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Added Pubsub to MongoDB flex template #75
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
Conversation
|
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 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 ℹ️ Googlers: Go here for more info. |
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
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.
What is the unit? Also specify default: 6000
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.
Do we want the default to be false? Probably better to set the default to true?
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.
Same, do we want to ignore by default?
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.
Set default to True?
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.
Unused Logger.
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.
Should we call this successful-json-conversion or something less ambiguous?
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.
Same here.
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.
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).
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.
The function will always get a valid json. Valid json will always get parsed as Bson documents
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.
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)?
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.
Missing javadoc.
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.
Can you reformat/reorder the imports?
f74503b to
ab3d32a
Compare
sabhyankar
left a comment
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.
LGTM
|
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 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 ℹ️ Googlers: Go here for more info. |
1782bbf to
ab3d32a
Compare
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
PiperOrigin-RevId: 285039202
* 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>
No description provided.