-
Notifications
You must be signed in to change notification settings - Fork 369
Fix the issue of unable to parse non-batch messages that with non-empty properties and empty payloads. #1435
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
base: master
Are you sure you want to change the base?
Conversation
…ty properties and empty payloads.
0a18a00 to
778835c
Compare
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.
Pull Request Overview
This PR fixes handling of non-batched messages with empty payloads but non-empty properties. Previously, the ReadMessage() function would return an error (ErrEOM) when encountering such messages, but the fix allows these messages to be properly read by returning an empty byte slice instead.
Key changes:
- Added
hasPropertiesfield toMessageReaderto track whether the message metadata contains properties - Modified
ReadMessage()to return[]byte{}(empty payload) instead ofErrEOMfor non-batched messages with properties and empty payloads - Updated existing test to verify the new behavior and added comprehensive end-to-end test
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pulsar/internal/commands.go | Added hasProperties field to track message properties; modified ReadMessage() to handle empty payloads with properties |
| pulsar/internal/commands_test.go | Updated test to verify empty payload is returned instead of ErrEOM for old format messages with properties |
| pulsar/reader_test.go | Added end-to-end test for empty payload messages with properties in non-batched mode |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if len(meta.Properties) > 0 { | ||
| r.hasProperties = true | ||
| } |
Copilot
AI
Nov 4, 2025
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.
The hasProperties field is never reset between message reads when a MessageReader is reused. If the same reader processes multiple messages and a message without properties follows one with properties, hasProperties will incorrectly remain true. Consider resetting this field in the ResetBuffer() method or at the start of ReadMessageMetadata().
| _, _, err = reader.ReadMessage() | ||
| assert.Equal(t, ErrEOM, err) | ||
| _, payload, _ = reader.ReadMessage() | ||
| assert.Equal(t, []byte{}, payload) |
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.
It's better to check the err is nil here.
Motivation
Modifications
Correct parse non-batch messages that with non-empty properties and empty payloads.
Verifying this change
(Please pick either of the following options)
This change is a trivial rework / code cleanup without any test coverage.
(or)
This change is already covered by existing tests, such as (please describe tests).
(or)
This change added tests and can be verified as follows:
(example:)
Does this pull request potentially affect one of the following parts:
If
yeswas chosen, please highlight the changesDocumentation