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

server: Add support for KEYS cmd to list topics #66

Merged
merged 2 commits into from
Sep 27, 2018
Merged

Conversation

papanikge
Copy link
Contributor

@papanikge papanikge commented Aug 30, 2018

This commit adds a new cmd in the likes of the redis KEYS command.
It is used to list all the topics of the cluster in their fully qualified names.

Note: Normal redis KEYS command takes one argument which matches
it as a glob against all keyspace. We do indeed accept an argument for
the sake of compatibility but we ignore it.

Usage example:

localhost:6380> keys topics:
1) "topics:greetings"
2) "topics:__consumer_offsets"
3) "topics:another-topic"

Copy link
Contributor

@dtheodor dtheodor left a comment

Choose a reason for hiding this comment

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

Looks fine

server.go Outdated
//
// KEYS
case "KEYS":
// Normally in redis proto KEYS has an argument but we ingore it.
Copy link
Contributor

Choose a reason for hiding this comment

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

For the sake of semantics, I believe we should only accept topics: as the argument. We do the same for DEL.

Copy link
Contributor Author

@papanikge papanikge Sep 13, 2018

Choose a reason for hiding this comment

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

Do you mean performing the command only in the case the first arg is topics:? ok sounds good, since we do it elsewhere too

break
}

var topic_names []interface{}
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason this isn't []string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It returns:

./server.go:169:36: cannot use topic_names (type []string) as type []interface {} in argument to writer.WriteObjects

I think it has to do with the internal implementation of interfaces, the only thing I found is this article and the relevant part is:

By running this, you can see that we encounter the following error: cannot use names (type []string) as type []interface {} in function argument. If we want to actually make that work, we would have to convert the []string to an []interface{}:

package main

import (
    "fmt"
)

func PrintAll(vals []interface{}) {
    for _, val := range vals {
        fmt.Println(val)
    }
}

func main() {
    names := []string{"stanley", "david", "oscar"}
    vals := make([]interface{}, len(names))
    for i, v := range names {
        vals[i] = v
    }
    PrintAll(vals)
}
Run it here:http://play.golang.org/p/Dhg1YS6BJS

That’s pretty ugly, but c'est la vie. Not everything is perfect. (in reality, this doesn’t come up very often, because []interface{} turns out to be less useful than you would initially expect)

At first I had the topic_names` var as a slice of strings as you say, and has something to "unwrap" the strings and convert them to interfaces but we discussed it a bit with @dtheodor and we converged on just using an interface slice.

Feel free to clarify it more for me, and/or suggest anything else, cause I'm a beginner in go :)

Copy link
Contributor

@agis agis Sep 14, 2018

Choose a reason for hiding this comment

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

Ah I see. Fair enough. A more detailed answer on this can be found here.

server.go Outdated
@@ -140,6 +140,34 @@ func (s *Server) handleConn(conn net.Conn) {
break
}
writeErr = writer.WriteObjects(stats.toRedis()...)
// Get all topics available
Copy link
Contributor

Choose a reason for hiding this comment

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

How about a simpler and more consistent with redis conventions:

List all topics

We should also use the same phrasing in the README.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed, will change.

rafka_test.go Outdated
}
if !found {
t.Errorf("Keys response did not contain expected 'topic:foo': %s", topics)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

You can simplify this by returning when/if the topic is found. Otherwise, fail. So no need for found.

@papanikge
Copy link
Contributor Author

@agis comments addressed.. :)

server.go Show resolved Hide resolved
@@ -170,6 +170,9 @@ func (s *Server) handleConn(conn net.Conn) {

if len(topic_names) > 0 {
writeErr = writer.WriteObjects(topic_names...)
} else {
// we need to return empty array here
_, writeErr = writer.Write([]byte{'*', '0', '\r', '\n'})
Copy link
Contributor

Choose a reason for hiding this comment

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

As a sidenote, we can just do a WriteObjects() when secmask/go-redisproto#2 is merged.

@agis
Copy link
Contributor

agis commented Sep 20, 2018

@papanikge Can you please rebase on top of master?

@papanikge
Copy link
Contributor Author

@agis rebased and squashed. sorry for the delay..

@dtheodor
Copy link
Contributor

The test failure seems legit

@papanikge
Copy link
Contributor Author

oh yes, that' mine I forgot to change the test to account for the new prefix, sorry about that. On it.

@agis agis merged commit 3155427 into master Sep 27, 2018
@agis agis deleted the implement-keys-cmd branch September 27, 2018 12:18
agis added a commit that referenced this pull request Sep 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants