-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Decompression payload if needed in KeyShared subscription #7416
Conversation
@codelipenghui Oh, great. it is pretty fast. |
@feeblefakie it's introduced by #7107. |
@codelipenghui Thank you ! 🙇♂️ |
// If the message was part of a batch (eg: a batch of 1 message), we need | ||
// to read the key from the first single-message-metadata entry | ||
PulsarApi.CompressionType compressionType = metadata.getCompression(); | ||
CompressionCodec codec = CompressionCodecProvider.getCompressionCodec(compressionType); |
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.
We should avoid decompressing in the broker in any case. This is very dangerous from a CPU usage point of view.
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.
I'm not sure how to handle #7107 that fixed. Maybe we can use the first single message key as the batch message key, but this requires all clients catchup.
So, I think we can onboard this PR first ensure correctness, and I will create an issue for the clients catchup work.
And I have add check if (!metadata.hasOrderingKey() && !metadata.hasPartitionKey() && metadata.hasNumMessagesInBatch())
, if the clients set the key of the batch message to the key of the first message, it can reduce overhead.
I have test with the KeyBasedBatcher. It already set the batch message key.
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.
A better way would be to ensure that producers (in every lang) are attaching the routing key on the message metadata protobuf (at the batch level). That is not compressed so we should always be safe.
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.
Make sense, so it's better to revert #7107 and other lang clients should attach the routing key on the message metadata.
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.
@merlimat I have addressed your comment, please take a look, thanks.
/pulsarbot run-failure-checks |
3 similar comments
/pulsarbot run-failure-checks |
/pulsarbot run-failure-checks |
/pulsarbot run-failure-checks |
/pulsarbot run-failure-checks |
1 similar comment
/pulsarbot run-failure-checks |
Decompression payload if needed in KeyShared subscription (cherry picked from commit ed3583a)
Decompression payload if needed in KeyShared subscription
Decompression payload if needed in KeyShared subscription (cherry picked from commit ed3583a)
Decompression payload if needed in KeyShared subscription
Decompression payload if needed in KeyShared subscription (cherry picked from commit ed3583a)
…tch builder - restore logic that was removed in apache#7416
Fixes #7414
Motivation
Decompression payload if needed in KeyShared subscription
Verifying this change
Unit test added
Does this pull request potentially affect one of the following parts:
If
yes
was chosen, please highlight the changesDocumentation