-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
PubsubMessageWithTopicCoder.of() is returning wrong coder #31619
base: master
Are you sure you want to change the base?
PubsubMessageWithTopicCoder.of() is returning wrong coder #31619
Conversation
Assigning reviewers. If you would like to opt out of this review, comment R: @m-trieu for label java. Available commands:
The PR bot will only process comments in the main thread (not review comments). |
@stankiewicz do you mind also cleaning up the one occurrence of |
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 java
Ah, wow. Thanks for the fix. Out of curiosity, did you actually run into a failure due to this ? |
Yes. It took a while to debug the issue:) |
Thanks LGTM. Seems like this was added in https://github.com/apache/beam/pull/26063/files cc: @reuvenlax in case we are missing something here. |
Shall we merge it? |
Reminder, please take a look at this pr: @m-trieu @chamikaramj |
Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment R: @robertwb for label java. Available commands:
|
Reminder, please take a look at this pr: @robertwb @ahmedabu98 |
PubsubMessageWithTopicCoder should return PubsubMessageWithTopicCoder PubsubMessageWithAttributesAndMessageIdCoder.
While investigating Dynamic Destinations on Direct runner I found out that PubsubMessageWithTopicCoder is never used and topic is lost and pipeline fails.
fixes #31679