Recently my corner of the Twitter-verse has been moderately aflame with a number of people pointing and laughing at a chunk of MongoDB source code; this example is typical. At first I automatically accepted that the code was laughable (and you have to admit that seeing a call to random() in database client code is odd) but when the link kept cropping up I got curious enough to start reading the surrounding code. Unfortunately the more I read the more sane the code looked. Since the details don't exactly fit in a tweet, it's time to write a more substantial explanation of the code and why it's sane.

Let's start with the code that people are directly laughing about (or so I assume since this is the line that people have been linking to):

if (!((_ok) ? true : (Math.random() > 0.1))) {
   return res;
}

In the interests of cleverness, someone has made this if condition much too hard to read. Rewritten to be clear, it is '!_ok && Math.random() <= 0.1'. If _ok is false, this if and its immediate return will fire about 10% of the time. If _ok is true, it will never fire.

Now let's look at the surrounding code, or at least bits of it. The context goes like this:

abstract class ConnectionStatus {
  ....
  static abstract class UpdatableNode {
    ....
    public CommandResult update() {
      CommandResult res = null;
      try {
        ... try to talk to a server ...

        if (!_ok) {
          getLogger().log(Level.INFO, "Server seen up: " + _addr);
        }
        _ok = true
        ...
      } catch(Exception e) {
        if (!((_ok) ? true : (Math.random() > 0.1))) {
          return res;
        }

        ... log a message about the
            the server being down ...
        _ok = false;
      }

      return res;
    }
...

Let me translate this: the effect of the if with the Math.random() is that if there's an error and the server has already been marked as down, about 10% of the time the code doesn't bother to log a message that it's still down (and doesn't record the actual exception caught as part of that log message). If the server is marked up and there's an error, a message is always logged. In all cases res is returned, possibly as null, and I believe no exception is propagated; I assume that this is an intended part of the API.

(There may actually be a bug here; it may be intended that the message is skipped 90% of the time when the server is already marked as down, not 10%.)

This code does not strike me as laughable, crazy, or worthy of mockery. It may have some defects and you may disagree with what it's doing, but it sure seems to be sane code in general. You probably have worse things in some of your projects (I know that I do in mine). My personal view is that what the code is doing is perfectly sensible to avoid flooding logs, but since I don't know MongoDB I don't know how much log volume this would produce (or avoid) and how important that log information would be during problem diagnosis.

(Note that the code will always log 'server seen down' and 'server seen up' when it first notices the server down and when it first notices the server back up. The only question is how many 'server still down' messages it logs or should log between those two, in a range somewhere between 'none' and 'one for every attempt to talk to the server'.)

If there is a fault in this code it is not the use of Math.random() in database client code, it is that the if condition has been written in such an overly clever way. And if people want to point and laugh at overly clever code, well, there's a lot of fish in a lot of barrels (our own generally included).

Update: C J Silverio pointed me to the initial commit for this code. The initial commit is much simpler and makes it clear that the current 'log 90%' state is a bug.