-
Notifications
You must be signed in to change notification settings - Fork 989
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
Added Pubsub to MongoDB flex template #75
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. |
|
||
void setBatchSizeBytes(Long batchSizeBytes); | ||
|
||
@Description("Maximum Connection idle time") |
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
void setMaxConnectionIdleTime(int maxConnectionIdleTime); | ||
|
||
@Description("Specify if SSL is enabled") | ||
@Default.Boolean(false) |
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?
void setSslEnabled(Boolean sslEnabled); | ||
|
||
@Description("Specify whether to ignore SSL certificate") | ||
@Default.Boolean(false) |
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?
void setWithOrdered(Boolean withOrdered); | ||
|
||
@Description("Enable invalidHostNameAllowed for ssl connection") | ||
@Default.Boolean(false) |
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?
*/ | ||
public static class ParseAsDocumentsFn extends DoFn<String, Document> { | ||
|
||
private static final Logger LOG = LoggerFactory.getLogger(ParseAsDocumentsFn.class); |
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.
|
||
private static final Counter successCounter = | ||
Metrics.counter( | ||
PubSubMessageToJsonDocument.class, "successful-messages-processed"); |
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?
|
||
private static final Counter failedCounter = | ||
Metrics.counter( | ||
PubSubMessageToJsonDocument.class, "failed-messages-processed"); |
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.
|
||
@ProcessElement | ||
public void processElement(ProcessContext context) { | ||
context.output(Document.parse(context.element())); |
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)?
import static org.hamcrest.CoreMatchers.equalTo; | ||
import static org.hamcrest.CoreMatchers.is; | ||
import static org.junit.Assert.assertThat; | ||
|
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.
|
||
package com.google.cloud.teleport.v2.templates; | ||
|
||
import com.google.cloud.teleport.v2.values.FailsafeElement; |
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
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.