FIX: Redis to handle event metadata. #3320
Conversation
Signed-off-by: Artur Souza <asouza.pro@gmail.com>
pubsub/redis/redis.go
Outdated
| metadataBytes := metadataValue.(string) | ||
| err := json.Unmarshal([]byte(metadataBytes), &metadata) | ||
| if err != nil { | ||
| r.logger.Warnf("Could not extract metadata for Redis message %s: %v", msg.ID, err) |
There was a problem hiding this comment.
This seems to be better than getting the message stuck because metadata could not be handled.
Signed-off-by: Artur Souza <asouza.pro@gmail.com>
|
I can create a specific issue if this needs further discussion. I created two issues that this PR is part of. Basically, bringing consistency across pubsub components regarding handling of pubsub metadata. This will only block our integration or E2E tests to reproduce the bug I am fixing for 1.13 - which is a P0 as it makes pubsub unusable for some components with gRPC subscriber. |
See the updated description and my comment. This fix can wait until 1.14 but we will not have E2E tests to validate the hotfix for 1.13 - which I can do manually. Of course, we can add yet-another-pubsub to E2E tests, but I don't think it is the right path since this is a basic inconsistency in pubsub that should be fixed (not P0). |
|
@artursouza my .02:
I'd prefer avoiding adding new features to a "tier 1" component (with a large user base like Redis) when we're already in endgame. But I can be open if other maintainers agree this is the best path forward. |
In-memory pubsub does not work for e2e because the subscriber is not the same app as the publisher. While the test can exist with another pubsub, I found it odd for such a basic feature not be supported by all beta and stable pubsub. If this behavior is not standard, then we should add a new column to the feature set of pubsub components. As you see, I opened an issue to discuss the expected behavior for pubsubs and see what others think. In the worst case, I can add this feature behind a component metadata flag for those that need it. |
|
@artursouza @yaron2 |
There was a problem hiding this comment.
I will approve this, but I really think you should add conformance tests.
I will not block on conformance tests, but that is the right thing to do.
My only blocking comment is to please prefix the error message with the component name as I have suggested. This helps with troubleshooting or grepping for similar logs (to see prevalence of an issue etc etc).
Beyond that I am fine merging this.
There was a problem hiding this comment.
Given that @berndverst and @yaron2 both agree to include this, I'm fine too. Thanks for your patience :)
Signed-off-by: Bernd Verst <github@bernd.dev>
|
I took the liberty to apply my suggested log prefix. LGTM now. |
Description
Redis does not handle event metadata at all. We had a P0 issue in runtime that was discovered because of Pulsar pubsub and it could not be reproduced with Redis because of inconsistency of handling event metadata - in this case, Redis simply drops it. This PR brings the Redis component (stable) at the same level of compatibility we should have across beta and stable pubsub components (IMO) regarding event metadata handling. This is not a P0 (IMO), it will just block the E2E test to reproduce the P0 bug we will need to fix.
Issue reference
We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.
Related to the following issues:
content-lengthheader dapr#7413Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list:
RELEASE_NOTE: FIXED Redis pubsub now handles event metadata.