Nom parser#129
Conversation
| '*' => self.parse_bulk(), | ||
| '-' => self.parse_error(), | ||
| _ => fail!((ErrorKind::ResponseError, "Invalid response when parsing value")), | ||
| let mut buf = [0; 4]; |
There was a problem hiding this comment.
I need to figure out the best way to read from the socket. This is just for testing locally now.
|
@badboy Sorry to ping you directly, but I am curious if you find this PR valuable? If so I will keep iterating on it. |
|
First, I appreciate your work here, regardless if we will merge it in the end. |
|
In any case I will keep working on it. I wanted to see if it would be a hard no or not :) No rush on the feedback by the way! Whenever is convenient. |
|
@gsquire I am personally curious about performance difference between the current parser and your nom based one. Any chance you can provide some numbers? |
|
@jwilm I don't have any numbers I am confident in now and am more focused on correctness. If it proves to be the same or worse then I will abandon this. |
|
I have to say...I am puzzled why reading from the socket causes the test to hang. The next step to try is a tcpdump of the traffic. |
| '-' => self.parse_error(), | ||
| _ => fail!((ErrorKind::ResponseError, "Invalid response when parsing value")), | ||
| let mut buf = vec![]; | ||
| match self.buf_reader.read_to_end(&mut buf) { |
There was a problem hiding this comment.
reading a stream to end will wait for an EOF, meaning for the connection to close.
That's definitely not what you want here.
You would want to read as much as possible, then try to parse it and if necessary read more of it.
There was a problem hiding this comment.
I am working on this now, and am trying to figure out the most efficient way to do so.
| Some(detail) => fail!((kind, desc, detail.to_string())), | ||
| None => fail!((kind, desc)), | ||
| fn parse_error(line: &str) -> RedisResult<Value> { | ||
| let desc = "An error was signaled by the server"; |
There was a problem hiding this comment.
Hm, okay, both spellings are ok. Don't mind then.
|
It's definitely off to a good start.
The current parser is pretty simple and not optimized at all. I'm sure there is some low-hanging fruit to speed it up. |
|
I need to read up on nom and why I am getting |
|
Another status update... It's just hanging on the transaction tests now. I know the tests are failing but I will attend to that once I am done figuring out why it's not reading all that it should. |
|
@badboy I haven't had much time to look at this as I just moved into a new place. I still plan on getting this working and will return to it in due time. If you have any free time, you can edit my PR too. :) |
|
The transaction tests are failing on this line: https://github.com/mitsuhiko/redis-rs/pull/129/files#diff-2c09afcdc3c420ab0678ba9b5e83959cR35 The read is blocking for some reason and I cannot figure out why. After this I will see what the parser errors are. |
gsquire
force-pushed
the
nom-parser
branch
2 times, most recently
from
40c393a to
9a4bc34
Compare
|
I have squashed and rebased my changes. For now I moved back to the old way of reading byte by byte since the buffered reader would just block for transactions. I would love to hear ideas as to why this could have been or what could be done to fix it. Also, there is a lot of refactoring I will get to as I haven't focused on code quality much to my chagrin. Let me know your thoughts and thanks for your patience with this PR. |
…is-rs#1092) Java: Update benchmarking to use CachedThreadPool (redis-rs#129) * Run with CachedThreadPool * Add cached thread pool with rejection handler * Remove unused constant --------- Signed-off-by: Andrew Carbonetto <andrew.carbonetto@improving.com>
3 participants
This is an extremely rough start to using nom. I am sharing it to get thoughts on the layout as well as how to debug the unit tests. Perhaps I have written the parsers in a completely wrong way.
If this seems like it will not fit into this library, then feel free to close it. Thanks for looking.
Related to #81