Scan issues

[lordnynex]

Hello,

I am using radix to do some analytics pipelining from a few redis instances. To accomplish this I'm using util.Scan to build up a queue of keys to export.

Unfortunately, I found out the hard way that util.Scan does not provide scan count hints during it's requests. This had the extremely time consuming side effect of being blazing fast in local development and extremely slow in production.

To work around this issue I've implemented util.Scan with slight modifications

func Scan(r redis.Cmder, ch chan string, cmd, key, pattern string, serverHint int) error {
  // ...
  args = append(args, cursor, "MATCH", pattern, "COUNT", serverHint)
}

This yields a huge performance increase because in large scale redis servers there is a high volume of empty cursors returned. I'm not quite sure yet how memory/key fragmentation applies to the cursors returned, but this issue is extremely difficult to detect on smaller instances.

For my case the 'non-optional' nature of serverHint is acceptable. If you choose to implement something similar, it may be better to set a sensible default?

[mediocregopher]

Hey there! Thanks for going in-depth with this, I must have had a brain fart when I made that method for not considering that someone would want to set the COUNT. I'll definitely be considering how best to get around this problem, most likely something along the lines of a ScanWithOpts method or something which takes in a struct instead of a thousand arguments, similar to what I did in cluster.

Another thing I noticed in the docs just now is this:

there is no need to use the same COUNT value for every iteration

So theoretically util.Scan could change the count as the scan goes on. I'm not sure based on what it would be doing this, something like number of empty keys returned per iteration? You've thought about this more than I have probably, I'm curious what metrics you were using when you increased the COUNT.

Thanks again!

[lordnynex]

@mediocregopher Thanks for the reply

So I think the large volume of scan iterations that yield empty results is a symptom of high volume or misconfigured high volume redis instances.

One thing to note is that currently, empty returns from redis scan cause util.Scan to write empty strings to the output channel. This isn't a huge problem to deal with but it was a surprising behavior. I have not looked if it's a result of zero values from var keys []string or zero values coming from List(). FWIW, I believe the redis protocol has some mechanism of signaling the return is empty.

It's common in any redis instance for the cursor to return a variable number of results, but as far as I understand, it's not possible to return a larger list than the counter hint? I'm not sure theres any need to detect these variances so long as a user specified hint can happen. I'm not sure there is benefit to adjusting the count hint from the client side. One possible use case is some sort of exponential backoff to speed up requests when a large number of consecutive empty cursors is encountered. To be honest, though, I don't see the benefit outweighing the effort of supporting that.

Regarding your question about metrics, it was a combination of pprof and tailing slow output logs that lead me to realize there is a problem. That lead me to use redis 'MONITOR' to actually watch the command pattern being sent to the server. For some reason in production without adding a count this yields either an empty set or 1 key per cursor call. As you can imagine that for hundreds of millions of keys it took quite awhile to export any meaningful amount of data. In order to understand this further I would have to understand Scan()'s loop state a bit better. Perhaps a Debug channel that has a 'loop canary' written to it in between scan calls. Otherwise, it's difficult to know how many keys are coming back per call.

I'm still trying to figure out if there is an optimal count to specify, but it seems use case dependent.

[mediocregopher]

One thing to note is that currently, empty returns from redis scan cause util.Scan to write empty strings to the output channel.

I definitely think that's a bug, although it's unfortunate that behavior's been in the package for so long it might have been picked up by someone as the "intended" behavior. I can't think of any way in which removing it would negatively affect anyone though.....

One possible use case is some sort of exponential backoff to speed up requests when a large number of consecutive empty cursors is encountered

That's more along the lines of what I was thinking. But if it's not worth it then bail.

Perhaps a Debug channel that has a 'loop canary' written to it in between scan calls. Otherwise, it's difficult to know how many keys are coming back per call.

I feel that at the point that I'm adding something like that in the user could just implement the SCAN loop themselves, the util.Scan stuff is more meant as a helper for the very general use-cases.

I'm going to think about the best way to add in a new method that allows count to be passed in. I'm always hesitant to add new things, so I might drag my feet a bit. Thanks again for bringing this up though!

[mediocregopher]

@lordnynex I've implemented the changes I talked about. I haven't merged them in yet, but if you want to check them out, the branch is called util-scan. Hopefully it works out for you :)

[mediocregopher]

@lordnynex sorry to nag, just wondering what your thoughts are on those changes. Thanks!

[lordnynex]

@mediocregopher Hey, sorry life's been busy. I just noticed the util-scan branch. I took a look at your commit and I like it. I'm still somewhat new to Go but I have grown to like the options struct approach. It seems to be the most flexible. Thanks for adding this.

Possible future enhancements (if a new version of radix is being planned):

• I've gotten some groans from the go-nuts about the way scan is using a string chan as an iterator. Future versions may want to implement a scan cursor struct. This may be a logical place to amend scan options mid-scan as you described previously.
• Since this options struct is much more flexible, it may be good to shift scan out of util and into client.

Thanks for getting this added so quickly. I will leave the issue open for you to close/tag however you see fit.

[mediocregopher]

Future versions will definitely have it implemented differently. I was mimicking what I saw other libraries doing at the time, namely the old etcd client library, but I didn't think it through well enough.

Thanks for looking through it though, I'll probably merge it in sometime today.

On Mon, May 23, 2016 at 6:03 PM Brandon Github notifications@github.com wrote:

@mediocregopher https://github.com/mediocregopher Hey, sorry life's been busy. I just noticed the util-scan branch. I took a look at your commit and I like it. I'm still somewhat new to Go but I have grown to like the options struct approach. It seems to be the most flexible. Thanks for adding this.

Possible future enhancements (if a new version of radix is being planned):

• I've gotten some groans from the go-nuts about the way scan is using a string chan as an iterator. Future versions may want to implement a scan cursor struct. This may be a logical place to amend scan options mid-scan as you described previously.
• Since this options struct is much more flexible, it may be good to shift scan out of util and into client.

Thanks for getting this added so quickly. I will leave the issue open for you to close/tag however you see fit.

— You are receiving this because you were mentioned.

Reply to this email directly or view it on GitHub https://github.com/mediocregopher/radix.v2/issues/25#issuecomment-221130963

[mediocregopher]

So I actually decided against merging what was there. I realized if I'm going to be making changes to util I might as well fix those race conditions if I can, instead of waiting for radix.v3. So now instead of ScanWithOpts I made a Scanner, which is an iterator struct, and deprecated the old Scan method. It's a pretty big change so I'm gonna sit on it for a little longer, but if you want to check it out @lordnynex I force pushed over the util-scan branch with it.

[mediocregopher]

Ok I merged in that PR, gonna close this, hope the changes work out for you!