Skip to content

Conversation

@pritukam
Copy link
Contributor

No description provided.

@googlebot
Copy link

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 Sep 27, 2019
@sabhyankar
Copy link
Member

@pritukam - Can you sign the CLA?

@pritukam
Copy link
Contributor Author

@googlebot I signed it!

@pritukam pritukam closed this Sep 27, 2019
@pritukam
Copy link
Contributor Author

@sabhyankar signed the CLA

@pritukam pritukam reopened this Sep 27, 2019
@sabhyankar sabhyankar self-requested a review October 2, 2019 15:37
@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 Oct 2, 2019
@chrlarsen
Copy link
Contributor

@sabhyankar the template has been updated to include udf, deadlettering and field extract support.

Copy link
Member

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.

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 just call these:
udf-transform-success-count
udf-transform-failed-count

Copy link
Contributor

Choose a reason for hiding this comment

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

Counters have been renamed.

Copy link
Member

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Udf has been added.

Copy link
Member

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 :)

Copy link
Contributor

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.

Copy link
Member

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

Copy link
Contributor

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.

Copy link
Member

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?

Copy link
Contributor

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.

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 check whether we can reuse the Gson object instead of creating a new one for every message?

Copy link
Member

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'

Comment on lines 180 to 181
Copy link
Member

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?

Comment on lines 224 to 225
Copy link
Member

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

@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 6, 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 6, 2019
@chrlarsen
Copy link
Contributor

@sabhyankar comments have been addressed. PR has been rebased onto PR-61 and commits have been squashed.

Copy link
Contributor

@chrlarsen chrlarsen left a comment

Choose a reason for hiding this comment

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

Added error converter.

@sabhyankar sabhyankar added the Google LGTM Approval of a pull request to be merged into the repository label Nov 8, 2019
sabhyankar added a commit that referenced this pull request Nov 18, 2019
PiperOrigin-RevId: 281057367
@sabhyankar sabhyankar merged commit 81e3e45 into GoogleCloudPlatform:master Nov 18, 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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants