Sets PubsubMessageWithAttributesAndMessageIdAndOrderingKeyCoder as default coder for all PubsubMessage #24887
Conversation
…fault coder for all PubsubMessage
| private static final Coder<String> MESSAGE_ID_CODER = StringUtf8Coder.of(); | ||
| // A message's messageId can only be null when the message is an outgoing message (i.e. to be | ||
| // published). Incoming messages will always have a non-null messageId | ||
| private static final Coder<String> MESSAGE_ID_CODER = NullableCoder.of(StringUtf8Coder.of()); |
There was a problem hiding this comment.
This is a breaking change as it adds a byte to the encoded message. Why is it needed?
There was a problem hiding this comment.
This would be needed to allow for use of this coder for outgoing messages, where the messageId field must be null to allow publishing.
|
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment |
|
@johnjcasey no I don’t believe so |
|
@damccorm i don’t believe it should be a blocker, no |
|
I notice this is attached to an open P1 for a while. What do we need to make progress? |
|
@kennknowles thanks for the bump. I have not had much bandwidth to meaningfully test this change. Something that concerns me and requires validation is whether or not the coder can be successfully set/changed from default so as to allow backward compatibility for pipeline authors. It appears that using .setCoder was not entirely sufficient based on prior exploration[1], but I have not yet revisited or gotten into a good repro-test feedback loop. I have no issue handing this off if it’s high priority and someone else is keen to take up the work. [1] #21162 (comment) |
|
This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions. |
|
This pull request has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time. |
This PR alters the default Coder for pubsub message objects from
PubsubMessageWithAttributesCodertoPubsubMessageWithAttributesAndMessageIdAndOrderingKeyCoderas default coder for allPubsubMessage.class. Note that there was a previous dev@ thread[1] on this subject where there was no opposition to this change.The one important detail to call out is that pipeline authors may need to explicitly set the coder to the value which was previously the default,
PubsubMessageWithAttributesCoder, in cases where pipeline update is required. I do have some concern over whether that's completely sufficient/possible based on the behaviour observed in #21162 (comment).I'm not sure this should be merged until further testing can be done to validate whether a running pipeline can be successfully updated.
Fixes #23525
[1] https://lists.apache.org/thread/c3qk0cp3rbhk2wh8m0z8gqxqy931dbwc
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
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, commentfixes #<ISSUE NUMBER>instead.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI.