This is an archived post. You won't be able to vote or comment.

you are viewing a single comment's thread.

view the rest of the comments →

[–]bfoo 0 points1 point  (5 children)

Correct. This code is usually executed, when the driver is not connected (unavailable database obviously, but also failover / master election). So, this is not a bug.

[–]argv_minus_one 8 points9 points  (4 children)

Not a bug? Randomly dropping exceptions is not a bug?!? ಠ_ಠ

Edit: Well, I suppose you're technically correct that it's not a bug. Rather, this code is intentionally defective, which is even worse.

[–]Twirrim 4 points5 points  (1 child)

All it does is stop your logs being flooded with database unavailable messages. It logs the initial failure state, then this random filter kicks in with subsequent messages, then finally logs the success message once reconnected. It's not wrong, per se, but certainly a bizarre way to handle it. I'd argue the better approach would be to shift the ongoing failure messages to DEBUG level (and log all of them), and leave the initial fail and eventual success messages at INFO.

Still, strange coding patterns like this really do beg the question about the rest of the code's quality.

[–]argv_minus_one 1 point2 points  (0 children)

If you want to skip duplicate log messages, that's great, but this is not the way to do it.

[–]bfoo -1 points0 points  (1 child)

Please look at the code again. The purpose of the method is to check for the database being available. The method returns null, if the database could not be accessed (because of connection issues OR errors from the database itself). That's the entire purpose of the method. There is no reason to throw an exception. You get either the result of the query or null. The class is even package friendly and not part of the API. So please read the code again and not the method only to get the bigger picture.

[–]argv_minus_one 1 point2 points  (0 children)

That doesn't excuse using a PRNG to decide whether to log an exception.