You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2022/08/17 23:35:32 UTC

[GitHub] [beam] dedocibula opened a new pull request, #22769: [BEAM-12164] Feat: Added support to Cloud Spanner Change Streams connector for including transaction tags in the Change Stream records

dedocibula opened a new pull request, #22769:
URL: https://github.com/apache/beam/pull/22769

   This change adds support for handling transaction tag fields, introduced to the output of Cloud Spanner Change Stream queries, in the Beam/Dataflow. If Cloud Spanner writes are relying on transaction tag feature for auditing, these transaction tags will be included in corresponding Change Stream records produced by the Change Streams tracking affected keyspace.
   
   Design doc: https://docs.google.com/document/d/1qA-PE8CcwNHonhnHG46kOgi7-b9tTnzaZ89daVbeYUU/edit
   
   ------------------------
   
   Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
   
    - [ ] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) and mention them in a comment (`R: @username`).
    - [ ] Mention the appropriate issue in your description (for example: `addresses #123`), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment `fixes #<ISSUE NUMBER>` instead.
    - [ ] Update `CHANGES.md` with noteworthy changes.
    - [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
   
   See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://beam.apache.org/contribute/get-started-contributing/#make-the-reviewers-job-easier).
   
   To check the build health, please visit [https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md](https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md)
   
   GitHub Actions Tests Status (on master branch)
   ------------------------------------------------------------------------------------------------
   [![Build python source distribution and wheels](https://github.com/apache/beam/workflows/Build%20python%20source%20distribution%20and%20wheels/badge.svg?branch=master&event=schedule)](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
   [![Python tests](https://github.com/apache/beam/workflows/Python%20tests/badge.svg?branch=master&event=schedule)](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
   [![Java tests](https://github.com/apache/beam/workflows/Java%20Tests/badge.svg?branch=master&event=schedule)](https://github.com/apache/beam/actions?query=workflow%3A%22Java+Tests%22+branch%3Amaster+event%3Aschedule)
   [![Go tests](https://github.com/apache/beam/workflows/Go%20tests/badge.svg?branch=master&event=schedule)](https://github.com/apache/beam/actions?query=workflow%3A%22Go+tests%22+branch%3Amaster+event%3Aschedule)
   
   See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] dedocibula commented on a diff in pull request #22769: [BEAM-12164] Feat: Added support to Cloud Spanner Change Streams connector for including transaction tags in the Change Stream records

Posted by GitBox <gi...@apache.org>.
dedocibula commented on code in PR #22769:
URL: https://github.com/apache/beam/pull/22769#discussion_r954184493


##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/changestreams/mapper/ChangeStreamRecordMapper.java:
##########
@@ -253,6 +255,8 @@ private DataChangeRecord toDataChangeRecord(
         valueCaptureTypeFrom(row.getString(VALUE_CAPTURE_TYPE_COLUMN)),
         row.getLong(NUMBER_OF_RECORDS_IN_TRANSACTION_COLUMN),
         row.getLong(NUMBER_OF_PARTITIONS_IN_TRANSACTION_COLUMN),
+        row.getString(TRANSACTION_TAG),

Review Comment:
   Yep, there will always be transaction tag field (might be empty string) in the output



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] pabloem merged pull request #22769: [BEAM-12164] Feat: Added support to Cloud Spanner Change Streams connector for including transaction tags in the Change Stream records

Posted by GitBox <gi...@apache.org>.
pabloem merged PR #22769:
URL: https://github.com/apache/beam/pull/22769


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] pabloem commented on pull request #22769: [BEAM-12164] Feat: Added support to Cloud Spanner Change Streams connector for including transaction tags in the Change Stream records

Posted by GitBox <gi...@apache.org>.
pabloem commented on PR #22769:
URL: https://github.com/apache/beam/pull/22769#issuecomment-1227835465

   Run Java PreCommit


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] ahmedabu98 commented on pull request #22769: [BEAM-12164] Feat: Added support to Cloud Spanner Change Streams connector for including transaction tags in the Change Stream records

Posted by GitBox <gi...@apache.org>.
ahmedabu98 commented on PR #22769:
URL: https://github.com/apache/beam/pull/22769#issuecomment-1224068662

   Hey @dedocibula, Can you please fix spotless errors?
   
   Also looks like `Java_GCP_IO_Direct` because the test in [`PostProcessingMetricsDoFnTest.java`](https://github.com/apache/beam/blob/6f704a15b4bda3f8842653e8485c96d43a3d2fe5/sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/spanner/changestreams/dofn/PostProcessingMetricsDoFnTest.java#L57) is using the old version of `DataChangeRecord`.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] pabloem commented on pull request #22769: [BEAM-12164] Feat: Added support to Cloud Spanner Change Streams connector for including transaction tags in the Change Stream records

Posted by GitBox <gi...@apache.org>.
pabloem commented on PR #22769:
URL: https://github.com/apache/beam/pull/22769#issuecomment-1228926908

   lgtm thanks!


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] pabloem commented on pull request #22769: [BEAM-12164] Feat: Added support to Cloud Spanner Change Streams connector for including transaction tags in the Change Stream records

Posted by GitBox <gi...@apache.org>.
pabloem commented on PR #22769:
URL: https://github.com/apache/beam/pull/22769#issuecomment-1227832211

   Run Java PostCommit


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] thiagotnunes commented on a diff in pull request #22769: [BEAM-12164] Feat: Added support to Cloud Spanner Change Streams connector for including transaction tags in the Change Stream records

Posted by GitBox <gi...@apache.org>.
thiagotnunes commented on code in PR #22769:
URL: https://github.com/apache/beam/pull/22769#discussion_r948555250


##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/changestreams/mapper/ChangeStreamRecordMapper.java:
##########
@@ -253,6 +255,8 @@ private DataChangeRecord toDataChangeRecord(
         valueCaptureTypeFrom(row.getString(VALUE_CAPTURE_TYPE_COLUMN)),
         row.getLong(NUMBER_OF_RECORDS_IN_TRANSACTION_COLUMN),
         row.getLong(NUMBER_OF_PARTITIONS_IN_TRANSACTION_COLUMN),
+        row.getString(TRANSACTION_TAG),

Review Comment:
   Are these always present or can they be `null`? If the latter is true, you have to do `row.isNull(TRANSACTION_TAG) ? null : row.getString(TRANSACTION_TAG)`



##########
sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/spanner/changestreams/mapper/ChangeStreamRecordMapperTest.java:
##########
@@ -99,6 +99,8 @@ public void testMappingUpdateStructRowToDataChangeRecord() {
             ValueCaptureType.OLD_AND_NEW_VALUES,
             10L,
             2L,
+            "transactionTag",

Review Comment:
   If the transaction tag could be null, could you add a test?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] thiagotnunes commented on a diff in pull request #22769: [BEAM-12164] Feat: Added support to Cloud Spanner Change Streams connector for including transaction tags in the Change Stream records

Posted by GitBox <gi...@apache.org>.
thiagotnunes commented on code in PR #22769:
URL: https://github.com/apache/beam/pull/22769#discussion_r949655824


##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/changestreams/mapper/ChangeStreamRecordMapper.java:
##########
@@ -253,6 +255,8 @@ private DataChangeRecord toDataChangeRecord(
         valueCaptureTypeFrom(row.getString(VALUE_CAPTURE_TYPE_COLUMN)),
         row.getLong(NUMBER_OF_RECORDS_IN_TRANSACTION_COLUMN),
         row.getLong(NUMBER_OF_PARTITIONS_IN_TRANSACTION_COLUMN),
+        row.getString(TRANSACTION_TAG),

Review Comment:
   Sorry, just to make sure I understand, there will always be a transaction tag?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] dedocibula commented on a diff in pull request #22769: [BEAM-12164] Feat: Added support to Cloud Spanner Change Streams connector for including transaction tags in the Change Stream records

Posted by GitBox <gi...@apache.org>.
dedocibula commented on code in PR #22769:
URL: https://github.com/apache/beam/pull/22769#discussion_r949375704


##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/changestreams/mapper/ChangeStreamRecordMapper.java:
##########
@@ -253,6 +255,8 @@ private DataChangeRecord toDataChangeRecord(
         valueCaptureTypeFrom(row.getString(VALUE_CAPTURE_TYPE_COLUMN)),
         row.getLong(NUMBER_OF_RECORDS_IN_TRANSACTION_COLUMN),
         row.getLong(NUMBER_OF_PARTITIONS_IN_TRANSACTION_COLUMN),
+        row.getString(TRANSACTION_TAG),

Review Comment:
   Yes these are always present now



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] pabloem commented on pull request #22769: [BEAM-12164] Feat: Added support to Cloud Spanner Change Streams connector for including transaction tags in the Change Stream records

Posted by GitBox <gi...@apache.org>.
pabloem commented on PR #22769:
URL: https://github.com/apache/beam/pull/22769#issuecomment-1227642463

   can you run `./gradlew :spotlessApply`? This will fix formatting issue (Spotless)


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] github-actions[bot] commented on pull request #22769: [BEAM-12164] Feat: Added support to Cloud Spanner Change Streams connector for including transaction tags in the Change Stream records

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #22769:
URL: https://github.com/apache/beam/pull/22769#issuecomment-1218737377

   Assigning reviewers. If you would like to opt out of this review, comment `assign to next reviewer`:
   
   R: @lukecwik for label java.
   R: @ahmedabu98 for label io.
   
   Available commands:
   - `stop reviewer notifications` - opt out of the automated review tooling
   - `remind me after tests pass` - tag the comment author after tests pass
   - `waiting on author` - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)
   
   The PR bot will only process comments in the main thread (not review comments).


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] dedocibula commented on a diff in pull request #22769: [BEAM-12164] Feat: Added support to Cloud Spanner Change Streams connector for including transaction tags in the Change Stream records

Posted by GitBox <gi...@apache.org>.
dedocibula commented on code in PR #22769:
URL: https://github.com/apache/beam/pull/22769#discussion_r949376132


##########
sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/spanner/changestreams/mapper/ChangeStreamRecordMapperTest.java:
##########
@@ -99,6 +99,8 @@ public void testMappingUpdateStructRowToDataChangeRecord() {
             ValueCaptureType.OLD_AND_NEW_VALUES,
             10L,
             2L,
+            "transactionTag",

Review Comment:
   It cannot be null, always set



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org