Skip to content
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

fix for nats_kv to be consistent in passing meta values along #215

Merged
merged 1 commit into from
Jan 16, 2025

Conversation

kmpm
Copy link
Contributor

@kmpm kmpm commented Jan 15, 2025

The nats_kv processor removed all meta values from message on succes. That is not consistent behaviour with other operations.

  • The operations get and get_revision now copy any pre existing meta values on success.
  • Test get and get_revision as well as one other operation for consistency.

Copy link
Collaborator

@gregfurman gregfurman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this PR! Have just added a small suggestion.

Think for completeness we should copy the original message instead of creating a new one -- preseving the original message's metadata and context. Open to suggestions here though.

Comment on lines 31 to 36
msg := service.NewMessage(entry.Value())
//nolint:errcheck
src.MetaWalkMut(func(key string, value any) error {
msg.MetaSetMut(key, value)
return nil
})
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would probably be easier to just set the bytes of the original message to the new content. That way we can retain all the original metadata without needing to construct a new message from the KV entry.

Suggested change
msg := service.NewMessage(entry.Value())
//nolint:errcheck
src.MetaWalkMut(func(key string, value any) error {
msg.MetaSetMut(key, value)
return nil
})
msg := src.Copy()
msg.SetBytes(entry.Value())

This way would also retain the original message's context -- and therefore its tracing ID.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather do it in a different way that mimics the workflow of the other operations. Se added commit

@kmpm kmpm marked this pull request as draft January 16, 2025 14:03
@kmpm kmpm marked this pull request as ready for review January 16, 2025 14:12
Copy link
Collaborator

@gregfurman gregfurman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Will merge when CI passes 😄

@gregfurman gregfurman merged commit 9f2c9bb into warpstreamlabs:main Jan 16, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants