Fix the issue of unable to parse non-batch messages that with non-empty properties and empty payloads. #1435
Conversation
…ty properties and empty payloads.
coderzc
force-pushed
the
fix_message_parse_err
branch
from
0a18a00 to
778835c
Compare
There was a problem hiding this comment.
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 | ||
| } |
There was a problem hiding this comment.
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.
It's better to check the err is nil here.
2 participants
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