[Issue #1418] Fix: Properties not getting consistently set on pulsaradmin subscription message responses#1419
Conversation
…ion message responses
There was a problem hiding this comment.
Pull Request Overview
This PR fixes inconsistent property setting on messages returned by PeekMessages. Previously, properties from headers weren't being consistently set because the function would exit early when processing batch messages, potentially skipping some headers.
- Replaced early return with a flag to defer batch processing until all headers are processed
- Ensured all properties are consistently set on messages regardless of header processing order
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
lhotari
left a comment
lhotari
left a comment
There was a problem hiding this comment.
LGTM. Thanks for the contribution! I wonder if a test could be added to catch possible regressions in the future?
|
Great fix, could you please add some unit tests? |
|
I absolutely can, I wasn't sure what style was sufficient I've made a basic addition to an existing test, which should catch it
I could write a dedicated test, but I wasn't sure how to go about asserting the time is as expected in a robust way |
|
OK I ended up updating TestPeekMessageWithProperties with a batched/non-batched variant which should well cover this scenario.
I'm not sure if you'll like the style, I could split them into separate tests but from a functionality point of view I think it'll work. I had to use the same "cheat" as the other batched unit test, but I've made it more explicit / not fail if you pause it in a debugger. Let me know if you want something different :) . |
…line (#4453) This will: - Expose metrics to show how far behind each partition of the topic being consume is in ms - We utilise peekMessages on the pulsar REST/admin api to get this information - It is optional and off by default The purpose of this is so you can see how far behind the ingester is in time (i.e 3 minutes) - pulsar only exposes backlog in number of messages, so its hard to tell how much impact a delay is having (i.e 1000 messages could be 1 second behind or 10 minutes) Also exposes a metric that shows the publish time of messages being processed by the pipeline for each partition **Note** I've had to "fork" some of pulsar-client-go due to a bug in the client. This is being fixed in apache/pulsar-client-go#1419. Once that is in the upstream client we can remove this internal code copy/fork --------- Signed-off-by: JamesMurkin <jamesmurkin@hotmail.com>
Motivation
The properties on messages returned by PeekMessages is not returned consistently as described in #1418
This change makes sure we set all properties from headers before continuing on, so the returned message should consistently contain all properties.
Currently the properties returned is inconsistent on repeated calls, even if the message returned is the same each time.
Modifications
I've made it so all headers are processed / al properties are set on every message, rather than exiting the loop early which can cause inconsistent results.
Verifying this change
This change is a trivial rework / code cleanup without any test coverage.
Does this pull request potentially affect one of the following parts:
Documentation