Issue 265: Remove filterKeysByType step when getting data from redis
#266
Conversation
Instead of filtering keys by type beforehand, assume all keys are of the expected type and handle exceptions later if we encounter a wrong type.
mayankasthana
force-pushed
the
265-remove-filter-by-key
branch
from
6f51d87 to
bc4be3f
Compare
mayankasthana
changed the title
filterKeysByType step when getting data from redisfilterKeysByType step when getting data from redis
| // Throw any other JedisDataException we encounter | ||
| case (_, e: JedisDataException) => throw e | ||
| // Throw any other Exception we encounter | ||
| case (_, e: Throwable) => throw e |
There was a problem hiding this comment.
@fe2s changed this. Please review this change again.
There was a problem hiding this comment.
Thanks @mayankasthana . Can we please also extract this parsing into the ParseUtils? I would keep this logic together in a single class, so it will easier to make any changes in future. Other than that it looks good.
| // Throw any other JedisDataException we encounter | ||
| case (_, e: JedisDataException) => throw e | ||
| // Throw any other Exception we encounter | ||
| case (_, e: Throwable) => throw e |
There was a problem hiding this comment.
Thanks @mayankasthana . Can we please also extract this parsing into the ParseUtils? I would keep this logic together in a single class, so it will easier to make any changes in future. Other than that it looks good.
mayankasthana
force-pushed
the
265-remove-filter-by-key
branch
from
25311de to
01cc519
Compare
| val res = stringKeys.zip(response).iterator.asInstanceOf[Iterator[(String, String)]] | ||
|
|
||
| val res = nodeKeys.zip(response) | ||
| .view |
There was a problem hiding this comment.
Thanks @mayankasthana for the changes!
One minor question - what is the reason we need view here?
There was a problem hiding this comment.
We don't explicitly need it. We are doing two flatMap operations, so I think it is creating two full collections, and we don't really need the intermediate collection. I was trying to make it lazy.
There was a problem hiding this comment.
I see. Can we replace .view with .iterator and remove .iterator from the last line?
There was a problem hiding this comment.
Yes, we can do that.
But now that I think about it, I think I changed the semantics. Earlier we used to give out an Iterator of a strict collection and I changed it to become lazy. I think doing .force.iterator or .collect.iterator at the end is better, so that we evaluate the whole collection before giving out the Iterator. This way, we also throw any exceptions immediately and not need to wait for .next to be called by the caller.
There was a problem hiding this comment.
I just removed the view thing to not make things complicated.
|
Yay! Thanks @fe2s! |
#265
Instead of filtering keys by type beforehand, assume all keys are of the expected type and ignore exceptions later if we encounter a wrong type.