Re: [PR] BUG FIX : fix-ReadFromKafkaViaSDF-bug-shall-set-coder [beam]

[via GitHub]

yyfhust commented on PR #34505:
URL: https://github.com/apache/beam/pull/34505#issuecomment-2798355259

   @scwhittle 
   
   Thanks for the feedback! integration test has been added.
   
   - Before the bug fix in this PR, the test failed as expected; I received the 
same error we saw before (see description).
   - After the bug fix in this PR, the test passed.
   
   I also ran our pipeline in Dataflow using the changes in this PR and 
confirmed that it fixed the issue I faced.
   
   please review. 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-unsubscr...@beam.apache.org

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

Re: [PR] BUG FIX : fix-ReadFromKafkaViaSDF-bug-shall-set-coder [beam]

[via GitHub]

yyfhust commented on PR #34505:
URL: https://github.com/apache/beam/pull/34505#issuecomment-2771265713

   .take-issue


-- 
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-unsubscr...@beam.apache.org

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

Re: [PR] BUG FIX : fix-ReadFromKafkaViaSDF-bug-shall-set-coder [beam]

[via GitHub]

johnjcasey commented on PR #34505:
URL: https://github.com/apache/beam/pull/34505#issuecomment-2775964623

   This change is technically not backwards compatible, because it removes the 
old, but public facing, methods on ReadSourceDescriptors. Can you keep those 
methods?


-- 
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-unsubscr...@beam.apache.org

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

Re: [PR] BUG FIX : fix-ReadFromKafkaViaSDF-bug-shall-set-coder [beam]

[via GitHub]

scwhittle commented on PR #34505:
URL: https://github.com/apache/beam/pull/34505#issuecomment-2778002871

   I believe that @johnjcasey was concerned you were removing public methods. I 
was responding to his comment, pointing out that I don't think any public 
methods were removed they were just  moved within the file to group them 
together with similar methods.  For example, `withKeyDeserializerAndCoder` was 
not removed it was moved next to `withKeyDeserializer`


-- 
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-unsubscr...@beam.apache.org

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

Re: [PR] BUG FIX : fix-ReadFromKafkaViaSDF-bug-shall-set-coder [beam]

[via GitHub]

yyfhust commented on PR #34505:
URL: https://github.com/apache/beam/pull/34505#issuecomment-2776147378

   stop reviewer notifications


-- 
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-unsubscr...@beam.apache.org

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

Re: [PR] BUG FIX : fix-ReadFromKafkaViaSDF-bug-shall-set-coder [beam]

[via GitHub]

yyfhust commented on PR #34505:
URL: https://github.com/apache/beam/pull/34505#issuecomment-2776657726

   ssign set of reviewers


-- 
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-unsubscr...@beam.apache.org

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

Re: [PR] BUG FIX : fix-ReadFromKafkaViaSDF-bug-shall-set-coder [beam]

[via GitHub]

yyfhust commented on PR #34505:
URL: https://github.com/apache/beam/pull/34505#issuecomment-2776179834

   > I think they are still there just grouped together differently.
   > […](#)
   > On Thu, Apr 3, 2025, 4:23 PM johnjcasey ***@***.***> wrote: This change is 
technically not backwards compatible, because it removes the old, but public 
facing, methods on ReadSourceDescriptors. Can you keep those methods? — Reply 
to this email directly, view it on GitHub <[#34505 
(comment)](https://github.com/apache/beam/pull/34505#issuecomment-2775964623)>, 
or unsubscribe 

 . You are receiving this because your review was requested.Message ID: 
***@***.***> [image: johnjcasey]*johnjcasey* left a comment 
([apache/beam#34505](https://github.com/apache/beam/pull/34505)) <[#34505 
(comment)](https://github.com/apache/beam/pull/34505#issuecomment-2775964623)> 
This change is technically not backwards compatible, because it removes the 
old, but public facing, methods on ReadSourceDescriptors. Can you keep those 
methods? — Repl
 y to this email directly, view it on GitHub <[#34505 
(comment)](https://github.com/apache/beam/pull/34505#issuecomment-2775964623)>, 
or unsubscribe 

 . You are receiving this because your review was requested.Message ID: 
***@***.***>
   
   sorry i do not get what do you mean here ; would you mind giving more 
details 


-- 
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-unsubscr...@beam.apache.org

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

Re: [PR] BUG FIX : fix-ReadFromKafkaViaSDF-bug-shall-set-coder [beam]

[via GitHub]

github-actions[bot] commented on PR #34505:
URL: https://github.com/apache/beam/pull/34505#issuecomment-2776151154

   Stopping reviewer notifications for this pull request: requested by 
reviewer. If you'd like to restart, comment `assign set of reviewers`


-- 
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-unsubscr...@beam.apache.org

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

Re: [PR] BUG FIX : fix-ReadFromKafkaViaSDF-bug-shall-set-coder [beam]

[via GitHub]

scwhittle commented on PR #34505:
URL: https://github.com/apache/beam/pull/34505#issuecomment-2776038688

   I think they are still there just grouped together differently.
   
   On Thu, Apr 3, 2025, 4:23 PM johnjcasey ***@***.***> wrote:
   
   > This change is technically not backwards compatible, because it removes
   > the old, but public facing, methods on ReadSourceDescriptors. Can you keep
   > those methods?
   >
   > —
   > Reply to this email directly, view it on GitHub
   > , or
   > unsubscribe
   > 

   > .
   > You are receiving this because your review was requested.Message ID:
   > ***@***.***>
   > [image: johnjcasey]*johnjcasey* left a comment (apache/beam#34505)
   > 
   >
   > This change is technically not backwards compatible, because it removes
   > the old, but public facing, methods on ReadSourceDescriptors. Can you keep
   > those methods?
   >
   > —
   > Reply to this email directly, view it on GitHub
   > , or
   > unsubscribe
   > 

   > .
   > You are receiving this because your review was requested.Message ID:
   > ***@***.***>
   >
   


-- 
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-unsubscr...@beam.apache.org

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

Re: [PR] BUG FIX : fix-ReadFromKafkaViaSDF-bug-shall-set-coder [beam]

[via GitHub]

yyfhust commented on PR #34505:
URL: https://github.com/apache/beam/pull/34505#issuecomment-2775661324

   > Thanks! Would it be possible to extend the test coverage? I think you 
could modify a simpler test like testKafkaIOWriteWithErrorHandler in 
KafkaIOIT.java to have a custom deserializer (perhaps just a no-op subclass of 
StringSerializer for example) without a registered coder. That would help 
prevent regressions as well.
   
   Yes, will do. Actually I was waiting for someone to point me to the place 
where I can write unit/integration tests 🤣  . 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-unsubscr...@beam.apache.org

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

Re: [PR] BUG FIX : fix-ReadFromKafkaViaSDF-bug-shall-set-coder [beam]

[via GitHub]

yyfhust commented on PR #34505:
URL: https://github.com/apache/beam/pull/34505#issuecomment-2773884667

   Tested this on the pipeline which raised error.  confirmed that the change 
fixed the error . No error raised when run pipeline on this PR 


-- 
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-unsubscr...@beam.apache.org

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

Re: [PR] BUG FIX : fix-ReadFromKafkaViaSDF-bug-shall-set-coder [beam]

[via GitHub]

yyfhust commented on code in PR #34505:
URL: https://github.com/apache/beam/pull/34505#discussion_r2025256262


##
sdks/java/io/kafka/src/main/java/org/apache/beam/sdk/io/kafka/KafkaIO.java:
##
@@ -1784,8 +1784,10 @@ public PCollection> expand(PBegin 
input) {
 .withConsumerConfigOverrides(kafkaRead.getConsumerConfig())
 
.withOffsetConsumerConfigOverrides(kafkaRead.getOffsetConsumerConfig())
 .withConsumerFactoryFn(kafkaRead.getConsumerFactoryFn())
-
.withKeyDeserializerProvider(kafkaRead.getKeyDeserializerProvider())
-
.withValueDeserializerProvider(kafkaRead.getValueDeserializerProvider())
+.withKeyDeserializerProviderAndCoder(
+kafkaRead.getKeyDeserializerProvider(), keyCoder)
+.withValueDeserializerProviderAndCoder(
+kafkaRead.getValueDeserializerProvider(), valueCoder)

Review Comment:
   this is at the entry of kafka IO, and it will 100% return a coder : 
   
https://github.com/apache/beam/blob/800d434e862c3ea48c74a38a6ab96387ed06fbf1/sdks/java/io/kafka/src/main/java/org/apache/beam/sdk/io/kafka/KafkaIO.java#L1940-L1943



-- 
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-unsubscr...@beam.apache.org

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

Re: [PR] BUG FIX : fix-ReadFromKafkaViaSDF-bug-shall-set-coder [beam]

[via GitHub]

yyfhust commented on code in PR #34505:
URL: https://github.com/apache/beam/pull/34505#discussion_r2025256262


##
sdks/java/io/kafka/src/main/java/org/apache/beam/sdk/io/kafka/KafkaIO.java:
##
@@ -1784,8 +1784,10 @@ public PCollection> expand(PBegin 
input) {
 .withConsumerConfigOverrides(kafkaRead.getConsumerConfig())
 
.withOffsetConsumerConfigOverrides(kafkaRead.getOffsetConsumerConfig())
 .withConsumerFactoryFn(kafkaRead.getConsumerFactoryFn())
-
.withKeyDeserializerProvider(kafkaRead.getKeyDeserializerProvider())
-
.withValueDeserializerProvider(kafkaRead.getValueDeserializerProvider())
+.withKeyDeserializerProviderAndCoder(
+kafkaRead.getKeyDeserializerProvider(), keyCoder)
+.withValueDeserializerProviderAndCoder(
+kafkaRead.getValueDeserializerProvider(), valueCoder)

Review Comment:
   this is at the entry of kafka IO, and it will 100% return a coder : 
   
https://github.com/apache/beam/blob/800d434e862c3ea48c74a38a6ab96387ed06fbf1/sdks/java/io/kafka/src/main/java/org/apache/beam/sdk/io/kafka/KafkaIO.java#L1940-L1943
   
   if coder is given, it will return the coder specified by user. If not, it 
will attempt to retrieve a coder from registry , which only has coder for 
build-in deserializer.  



-- 
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-unsubscr...@beam.apache.org

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

Re: [PR] BUG FIX : fix-ReadFromKafkaViaSDF-bug-shall-set-coder [beam]

[via GitHub]

yyfhust commented on code in PR #34505:
URL: https://github.com/apache/beam/pull/34505#discussion_r2024741168


##
sdks/java/io/kafka/src/main/java/org/apache/beam/sdk/io/kafka/KafkaIO.java:
##
@@ -1784,8 +1784,10 @@ public PCollection> expand(PBegin 
input) {
 .withConsumerConfigOverrides(kafkaRead.getConsumerConfig())
 
.withOffsetConsumerConfigOverrides(kafkaRead.getOffsetConsumerConfig())
 .withConsumerFactoryFn(kafkaRead.getConsumerFactoryFn())
-
.withKeyDeserializerProvider(kafkaRead.getKeyDeserializerProvider())
-
.withValueDeserializerProvider(kafkaRead.getValueDeserializerProvider())
+.withKeyDeserializerProviderAndCoder(
+kafkaRead.getKeyDeserializerProvider(), keyCoder)
+.withValueDeserializerProviderAndCoder(
+kafkaRead.getValueDeserializerProvider(), valueCoder)

Review Comment:
   keyCoder and valueCoder must be non-nullable. they are resolved here  
https://github.com/apache/beam/blob/800d434e862c3ea48c74a38a6ab96387ed06fbf1/sdks/java/io/kafka/src/main/java/org/apache/beam/sdk/io/kafka/KafkaIO.java#L1562-L1563
 



-- 
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-unsubscr...@beam.apache.org

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

Re: [PR] BUG FIX : fix-ReadFromKafkaViaSDF-bug-shall-set-coder [beam]

[via GitHub]

yyfhust commented on code in PR #34505:
URL: https://github.com/apache/beam/pull/34505#discussion_r2024741168


##
sdks/java/io/kafka/src/main/java/org/apache/beam/sdk/io/kafka/KafkaIO.java:
##
@@ -1784,8 +1784,10 @@ public PCollection> expand(PBegin 
input) {
 .withConsumerConfigOverrides(kafkaRead.getConsumerConfig())
 
.withOffsetConsumerConfigOverrides(kafkaRead.getOffsetConsumerConfig())
 .withConsumerFactoryFn(kafkaRead.getConsumerFactoryFn())
-
.withKeyDeserializerProvider(kafkaRead.getKeyDeserializerProvider())
-
.withValueDeserializerProvider(kafkaRead.getValueDeserializerProvider())
+.withKeyDeserializerProviderAndCoder(
+kafkaRead.getKeyDeserializerProvider(), keyCoder)
+.withValueDeserializerProviderAndCoder(
+kafkaRead.getValueDeserializerProvider(), valueCoder)

Review Comment:
   keyCoder and valueCoder must be non-nullable. they are resolved here  
https://github.com/apache/beam/blob/800d434e862c3ea48c74a38a6ab96387ed06fbf1/sdks/java/io/kafka/src/main/java/org/apache/beam/sdk/io/kafka/KafkaIO.java#L1562-L1563
 
   
   either from user input or infer from coderegistry



-- 
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-unsubscr...@beam.apache.org

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

Re: [PR] BUG FIX : fix-ReadFromKafkaViaSDF-bug-shall-set-coder [beam]

[via GitHub]

github-actions[bot] commented on PR #34505:
URL: https://github.com/apache/beam/pull/34505#issuecomment-2771322628

   Assigning reviewers. If you would like to opt out of this review, comment 
`assign to next reviewer`:
   
   R: @chamikaramj for label java.
   R: @Abacn for label build.
   R: @sjvanrossum for label kafka.
   
   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-unsubscr...@beam.apache.org

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