-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
add parsing for stream entries #557
base: master
Are you sure you want to change the base?
Conversation
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.
Thanks for looking at this.
I've done a quick pass and adding some initial feedback
See commit 4a86003. |
Is this working for XREAD or XREADGROUP? I wonder this is not working well for them because the nested structure is different, sorry if this is out of scope. |
You raise a valid question @tk42 it does look like this will only work for Any thoughts @smotes ? |
Hi @stevenh. I propose we keep things as they stand. The Redis docs distinguish between entries and stream names in return types, so I felt it on the caller to parse out the reply in parts and know where to find the nested entries within. Consider the
And here's a little code snippet showing how to parse that as is (adapted from some code in a personal project). reply, err := sc.conn.Do("XREADGROUP", "GROUP", "some-group-name", "some-consumer-name",
"BLOCK", 3000, "COUNT", 10, "STREAMS", "some-stream-name", ">")
vs, err := redis.Values(reply, err)
if err != nil {
if errors.Is(err, redis.ErrNil) {
continue
}
return err
}
// Get the first and only value in the array since we're only
// reading from one stream "some-stream-name" here.
vs, err = redis.Values(vs[0], nil)
if err != nil {
return err
}
// Ignore the stream name as the first value as we already have
// that in hand! Just get the second value which is guaranteed to
// exist per the docs, and parse it as some stream entries.
entries, err := ParseEntries(vs[1], nil)
if err != nil {
return fmt.Errorf("error parsing entries: %w", err)
} |
That makes sense to me @tk42 does that answer address your concerns? |
Hi @smotes @stevenh
I’m feeling that users for type StreamEntry struct {
Stream string
Entries []Entry
} and its corresponded parsing function like this func StreamEntries(reply interface{}, err error) ([]StreamEntry, error) {
vss, err := redis.Values(reply, err)
if err != nil {
// error handling
return nil, fmt.Errorf("error parsing reply: %w", err)
}
var streamEntries []StreamEntry
for _, vs := range vss {
name, err := redis.String(vs[0], nil)
if err != nil {
// error handling
streamEntries = append(streamEntries, new(StreamEntry))
}
entries, err := ParseEntries(vs[1], nil)
if err != nil {
// error handling
streamEntries = append(streamEntries, new(StreamEntry))
}
streamEntries = append(streamEntries, StreamEntry{
Stream: name,
Entries: entries,
})
}
return streamEntries, nil
} |
This PR is related to issue #375 and adds a first pass at parsing replies containing stream entries into a struct.
I tested things against a Redis server v5.0.7, which is the first version containing the streams feature: https://redislabs.com/blog/redis-5-0-is-here/.