search

alphazero / jredis

Java Client and Connectors for Redis
http://code.google.com/p/jredis/
Apache License 2.0
315 stars 136 forks source link

FindBugs pass #45

Closed szegedi closed 13 years ago

szegedi commented 13 years ago

Hi,

we're working on adopting JRedis for some stuff we do at Twitter, and I started evaluating its code. I gave it a pass through FindBugs, and fixed most of the problems in the areas it indicated. The results are below. There was some dead code, and some downright problematic bugs, i.e. one infinite recursion.

Hope you'll integrate these changes upstream. I'll keep working on this, making sure it's robust enough under heavy usage volumes Twitter has.

Cheers, Attila.

alphazero commented 13 years ago

Hi Attila,

Thanks for the effort. I'm a bit distracted/busy at the moment with other matters: I have had to leave off development for 2.0 features due to prof. work load for the past couple of months. There is a bit of wip code that's not pushed to git yet. I don't wish to pry but if you can generally outline the feature set that you know you will utilize I can make a determination if whether it is best for you to cut lose now or not. Specifically, the main interfaces will get updated to support the remaining 2.0 feature set. If that is any way relevant to your usage scenario, be aware of that. Beyond that the changes will be in RI and you should be isolated from that.

That said, I reviewed your fixes in the pull request. Mostly they are OK but keep in mind the bulk is hitting dead code (which is why there weren't caught by the unit tests). I specifically want to review the ConnectionBase fix regarding the IOException (the note for commit is merely FYI for you). Can't believe I missed the assign in the static block! :))

/R

alphazero commented 13 years ago

Attila, Just pulled your repo and ran the test and I get 8 fails. Info and Substr across the board. Is this on your end or my end? Lets get this resolved and we'll roll in your updates.

/R

szegedi commented 13 years ago

Hi,

well, my repo has now progressed beyond just a FindBugs pass. I merged robey's changes - 'cause that's what we were using originally within Twitter anyway. Also, we recently upgraded to Redis 2.2.4 and it turns out it no longer supports correctly the older protocol version, so I also merged anthonylauzon's fork that adds the new protocol support.

I also had some object info test failures after merging robey's changes and Redis 2.0.x, but they went away with 2.2.4. I also had test failures with substr -1,0 - it now returns a byte[] of 0 length, instead of null. It might be an encoding change in the responses of the new protocol; I don't think there's a semantic difference between "new byte[0]" and "null" in this case, so I adjusted the tests.

With these changes, all tests now pass with Redis 2.2.4 on my end.

szegedi commented 13 years ago

Oh, also, my fork currently deviates somewhat from your codebase in that we (since robey's fork) adopted java.util.logging instead of the combination of Log4J and Apache Commons Logging. Honestly, if I can make a suggestion, you shouldn't be using Apache Commons Logging as your log abstraction framework; if you want to have implementation-independent logging, I'd suggest you go for SLF4J.

alphazero commented 13 years ago

Hi Attila,

Ok, will try that. (Any performance impact that I should be aware of in these changes?) Please confirm: all ok with 2.2.4 with no failures. A bit concerned that this may leave a bunch of folks stuck on 2.0.x ..

java.util.logging is fine with me. A bit annoying with the 2 line emits but I'm sure its configurable somewhere. (Intensely dislike sl4j for absolutely no good reason ;)

alphazero commented 13 years ago

ran against 2.2 and master and still break. Btw, I remember seeing Robey adding some funky custom cmds to the client. Are you guys using a vanilla redis or a twitter specific redis? Thanks. (Like your fractals!)

szegedi commented 13 years ago

Interesting… I'll recheck my test run; I just checked and I have everything committed. Robey didn't just add the commands to the client, but also worked with Salvatore to get them into Redis proper - those commands are standard in 2.2 Redis :-) (they were in in the 2.1 dev version too).

The fractals? Glad you like them. There was a time when I was spending lots of time with fractal generators when I was 17-18 years old; I'm happy I managed to discover those few beauties :-)

alphazero commented 13 years ago

The issue was on my end. I had apparently ran install sometime ago and forgot about it. It was running the bin under /usr/... Sorry about that!

Good to know about Robey's sync with Salvatore. We're good to go. Thank you! (and convey same to Robey, please).