¿Por qué el controlador MongoDB Java utiliza un generador de números aleatorios en un condicional?

211

Vi el siguiente código en este commit para el controlador de conexión Java de MongoDB , y al principio parece ser una broma de algún tipo. ¿Qué hace el siguiente código?

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

(EDITAR: el código se ha actualizado desde la publicación de esta pregunta)

Monstieur
fuente
13
¿Qué parte de esto te está confundiendo?
Oliver Charlesworth
44
Creo que es confuso. ¡Este código se ejecuta en un bloque catch!
Proviste
11
@MarkoTopolnik: ¿Lo es? Podría escribirse mucho más claramente como if (!ok || Math.random() < 0.1)(o algo similar).
Oliver Charlesworth
55
github.com/mongodb/mongo-java-driver/commit/… no eres el primero, mira el comentario a esa línea
msangel
3
@msangel Esos tipos parecen estar criticando la lógica, no el estilo de codificación.
Marko Topolnik

Respuestas:

279

Después de inspeccionar la historia de esa línea, mi conclusión principal es que ha habido una programación incompetente en el trabajo.

  1. Esa línea es enrevesada gratuitamente. La forma general

    a? true : b

    para boolean a, bes equivalente a la simple

    a || b
  2. La negación circundante y los paréntesis excesivos complican aún más las cosas. Teniendo en cuenta las leyes de De Morgan, es una observación trivial que este código equivale a

    if (!_ok && Math.random() <= 0.1)
      return res;
  3. El commit que originalmente introdujo esta lógica tenía

    if (_ok == true) {
      _logger.log( Level.WARNING , "Server seen down: " + _addr, e );
    } else if (Math.random() < 0.1) {
      _logger.log( Level.WARNING , "Server seen down: " + _addr );
    }

    : Otro ejemplo de codificación incompetente, pero observe la lógica inversa : aquí se registra el evento si_ok o en el 10% de otros casos, mientras que el código en 2. devuelve el 10% de las veces y registra el 90% de las veces. Entonces, el compromiso posterior arruinó no solo la claridad, sino la corrección misma.

    Creo que en el código que ha publicado podemos ver cómo el autor pretendía transformar el original de if-thenalguna manera literal en su negación requerida para principiosreturn condición . Pero luego se equivocó e insertó un "doble negativo" efectivo al revertir el signo de desigualdad.

  4. Dejando a un lado los problemas de estilo de codificación, el registro estocástico es una práctica bastante dudosa por sí sola, especialmente porque la entrada del registro no documenta su propio comportamiento peculiar. La intención es, obviamente, reducir las reformulaciones del mismo hecho: que el servidor está actualmente inactivo. La solución adecuada es registrar solo los cambios del estado del servidor, y no cada uno su observación, y mucho menos una selección aleatoria del 10% de tales observaciones. Sí, eso requiere un poco más de esfuerzo, así que veamos algunos.

Solo puedo esperar que toda esta evidencia de incompetencia, acumulada por la inspección de solo tres líneas de código , no hable equitativamente del proyecto en su conjunto, y que este trabajo se limpie lo antes posible.

Marko Topolnik
fuente
26
Además, este parece ser, por lo que puedo decir, el controlador oficial de Java 10gen para MongoDB, por lo que, además de tener una opinión sobre el controlador de Java, creo que me da una opinión sobre el código de MongoDB
Chris Travers
55
Excelente análisis de solo unas pocas líneas de código, ¡podría convertirlo en una pregunta de entrevista! Su cuarto punto es la verdadera clave de por qué hay algo fundamentalmente malo en este proyecto (los otros podrían ser descartados como errores desafortunados del programador).
Abel
1
@ChrisTravers Es es el piloto oficial mongo mongo java para.
Assylias
17

https://github.com/mongodb/mongo-java-driver/commit/d51b3648a8e1bf1a7b7886b7ceb343064c9e2225#commitcomment-3315694

Hace 11 horas por gareth-rees:

Presumiblemente, la idea es registrar solo 1/10 de las fallas del servidor (y así evitar el envío masivo de correo no deseado), sin incurrir en el costo de mantener un contador o temporizador. (¿Pero seguramente mantener un temporizador sería asequible?)

msangel
fuente
13
No para hacer trampas: 1/10 de las veces devolverá res, por lo que registrará las otras 9/10 veces.
Supericy
23
@Supericy Eso definitivamente no es una trampa. Eso es aún más evidencia de las terribles prácticas de codificación de esta persona.
Anorov
7

Agregue un miembro de clase inicializado a negativo 1:

  private int logit = -1;

En el bloque de prueba, haga la prueba:

 if( !ok && (logit = (logit + 1 ) % 10)  == 0 ) { //log error

Esto siempre registra el primer error, luego cada décimo error posterior. Los operadores lógicos "cortocircuitan", por lo que logit solo se incrementa en un error real.

Si desea el primero y el décimo de todos los errores, independientemente de la conexión, haga que la clase logit sea estática en lugar de un miembro.

Como se señaló, esto debería ser seguro para subprocesos:

private synchronized int getLogit() {
   return (logit = (logit + 1 ) % 10);
}

En el bloque de prueba, haga la prueba:

 if( !ok && getLogit() == 0 ) { //log error

Nota: No creo que descartar el 90% de los errores sea una buena idea.

tpdi
fuente
1

He visto este tipo de cosas antes.

Había un código que podía responder ciertas 'preguntas' que provenían de otro código 'caja negra'. En el caso de que no pudiera responderlos, los reenviaría a otra pieza de código de 'caja negra' que era realmente lenta.

Entonces, a veces aparecían nuevas 'preguntas' nunca antes vistas, y se mostraban en un lote, como 100 de ellas seguidas.

El programador estaba contento con el funcionamiento del programa, pero quería alguna forma de mejorar el software en el futuro, si se descubrían posibles nuevas preguntas.

Entonces, la solución fue registrar preguntas desconocidas, pero resultó que había miles de preguntas diferentes. Los registros se hicieron demasiado grandes, y no hubo beneficio de acelerarlos, ya que no tenían respuestas obvias. Pero de vez en cuando, aparecían un montón de preguntas que podrían responderse.

Debido a que los registros se estaban volviendo demasiado grandes, y el registro estaba obstaculizando el registro de las cosas realmente importantes que obtuvo con esta solución:

Solo registre un 5% aleatorio, esto limpiará los registros, mientras que a la larga aún muestra qué preguntas / respuestas podrían agregarse.

Entonces, si ocurriera un evento desconocido, en una cantidad aleatoria de estos casos, se registraría.

Creo que esto es similar a lo que estás viendo aquí.

No me gustó esta forma de trabajar, por lo que eliminé este fragmento de código y simplemente registré estos mensajes en un archivo diferente , por lo que todos estaban presentes, pero no bloquearon el archivo de registro general.

Jens Timmerman
fuente
3
Excepto que estamos hablando de un controlador de base de datos aquí ... espacio de problema incorrecto, ¡OMI!
Steven Schlansker
@StevenSchlansker Nunca dije que esta fuera una buena práctica. Eliminé este fragmento de código y acabo de registrar estos mensajes en un archivo diferente.
Jens Timmerman