-
Notifications
You must be signed in to change notification settings - Fork 1.1k
PubSub to Elasticsearch dynamic template, along with refactoring for v2. #58
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
|
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 What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
|
@pritukam - Can you sign the CLA? |
|
@googlebot I signed it! |
|
@sabhyankar signed the CLA |
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
|
@sabhyankar the template has been updated to include udf, deadlettering and field extract support. |
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.
Since this file is being added in PR-61 as well, we will rebase after that PR is merged.
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 just call these:
udf-transform-success-count
udf-transform-failed-count
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.
Counters have been renamed.
v2/pubsub-to-elasticsearch/README.md
Outdated
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 want to mention that an optional UDF can also be applied.
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.
Udf has been added.
v2/pubsub-to-elasticsearch/README.md
Outdated
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 only want to read from a subscription. Please remove the option to read from a topic :)
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.
Topic functionality has been removed here and other references.
v2/pubsub-to-elasticsearch/README.md
Outdated
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.
host(s) since it could be a cluster
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.
host(s) has been added.
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.
I am not sure why we would use a stringified attribute map if the payload is empty?
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.
@sabhyankar I believe the original thinking was that if the payload is empty then there would be data in the attribute map. If the message is empty then we could throw an error and catch it in the deadletter table.
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 check whether we can reuse the Gson object instead of creating a new one for every message?
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 are only transforming and not necessarily 'filtering'
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.
Why not directly use FAILSAFE_ELEMENT_CODER instead of assigning it to failsafeElementCoder?
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.
See comment in the previous test
|
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. |
|
@sabhyankar comments have been addressed. PR has been rebased onto PR-61 and commits have been squashed. |
chrlarsen
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.
Added error converter.
PiperOrigin-RevId: 281057367
* 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.