Warum verwendet der MongoDB-Java-Treiber einen Zufallszahlengenerator in einer Bedingung?

211

Ich habe den folgenden Code in diesem Commit für den Java Connection-Treiber von MongoDB gesehen , und es scheint zunächst ein Witz zu sein. Was macht der folgende Code?

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

(BEARBEITEN: Der Code wurde seit dem Posten dieser Frage aktualisiert. )

Monstieur
quelle
13
Welcher Teil davon verwirrt Sie?
Oliver Charlesworth
4
Ich finde es verwirrend. Dieser Code wird in einem Catch-Block ausgeführt!
Proviste
11
@ MarkoTopolnik: Ist es? Es könnte viel klarer geschrieben werden als if (!ok || Math.random() < 0.1)(oder etwas Ähnliches).
Oliver Charlesworth
5
github.com/mongodb/mongo-java-driver/commit/… Sie sind nicht der Erste, siehe Kommentar zu dieser Zeile
msangel
3
@msangel Diese Leute scheinen die Logik zu kritisieren, nicht den Codierungsstil.
Marko Topolnik

Antworten:

279

Nachdem ich die Geschichte dieser Linie untersucht habe, ist meine Hauptschlussfolgerung, dass eine inkompetente Programmierung am Werk war.

  1. Diese Linie ist unentgeltlich verwickelt. Die allgemeine Form

    a? true : b

    denn boolean a, bist gleichbedeutend mit dem Einfachen

    a || b
  2. Die umgebende Verneinung und übermäßige Klammern verschlingen die Dinge weiter. Unter Berücksichtigung der Gesetze von De Morgan ist es eine triviale Beobachtung, dass dieser Code gleichkommt

    if (!_ok && Math.random() <= 0.1)
      return res;
  3. Das Commit, das ursprünglich diese Logik eingeführt hatte, hatte

    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 );
    }

    - Ein weiteres Beispiel für inkompetente Codierung, aber beachten Sie die umgekehrte Logik : Hier wird das Ereignis entweder _okoder in 10% der anderen Fälle protokolliert , während der Code in 2. 10% der Fälle zurückgibt und 90% der Fälle protokolliert. Das spätere Commit ruinierte also nicht nur die Klarheit, sondern auch die Korrektheit selbst.

    Ich denke, in dem Code, den Sie gepostet haben, können wir tatsächlich sehen, wie der Autor beabsichtigte, das Original if-thenbuchstäblich in seine Negation umzuwandeln, die für den frühen returnZustand erforderlich ist. Aber dann hat er es vermasselt und ein effektives "doppeltes Negativ" eingefügt, indem er das Ungleichheitszeichen umgekehrt hat.

  4. Abgesehen von Problemen mit dem Codierungsstil ist die stochastische Protokollierung an sich schon eine zweifelhafte Praxis, zumal der Protokolleintrag kein eigenes Verhalten dokumentiert. Die Absicht ist offensichtlich, Anpassungen derselben Tatsache zu reduzieren: dass der Server derzeit nicht verfügbar ist. Die geeignete Lösung besteht darin, nur Änderungen des Serverstatus und nicht jede seiner Beobachtungen zu protokollieren , geschweige denn eine zufällige Auswahl von 10% solcher Beobachtungen. Ja, das erfordert nur ein bisschen mehr Aufwand, also lasst uns einige sehen.

Ich kann nur hoffen, dass all diese Inkompetenzbeweise, die sich aus der Überprüfung von nur drei Codezeilen ergeben , nicht fair über das gesamte Projekt sprechen und dass diese Arbeit so schnell wie möglich bereinigt wird.

Marko Topolnik
quelle
26
Außerdem scheint dies, soweit ich das beurteilen kann, der offizielle 10gen Java-Treiber für MongoDB zu sein. Ich habe also nicht nur eine Meinung zum Java-Treiber, sondern auch eine Meinung zum Code von MongoDB
Chris Travers
5
Hervorragende Analyse von nur wenigen Codezeilen, ich könnte es einfach in eine Interviewfrage verwandeln! Ihr vierter Punkt ist der eigentliche Schlüssel, warum mit diesem Projekt etwas grundlegend nicht stimmt (die anderen könnten als unglückliche Programmiererfehler abgetan werden).
Abel
1
@ChrisTravers Es ist der offizielle Mongo Java-Treiber für Mongo.
Assylias
17

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

Vor 11 Stunden von gareth-rees:

Vermutlich besteht die Idee darin, nur etwa 1/10 der Serverausfälle zu protokollieren (und so ein massives Spammen des Protokolls zu vermeiden), ohne die Kosten für die Wartung eines Zählers oder Timers zu verursachen. (Aber sicherlich wäre es erschwinglich, einen Timer zu warten?)

msangel
quelle
13
Nicht zu picken, sondern: 1/10 der Zeit wird res zurückgegeben, so dass die anderen 9/10 mal protokolliert werden.
Supericy
23
@ Supericy Das ist definitiv kein Trottel. Dies ist nur ein weiterer Beweis für die schrecklichen Codierungspraktiken dieser Person.
Anorov
7

Fügen Sie ein Klassenmitglied hinzu, das auf negativ 1 initialisiert wurde:

  private int logit = -1;

Führen Sie im try-Block den folgenden Test durch:

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

Dies protokolliert immer den ersten Fehler und dann jeden zehnten nachfolgenden Fehler. Logische Operatoren "schließen" kurz, sodass die Protokollierung nur bei einem tatsächlichen Fehler erhöht wird.

Wenn Sie den ersten und zehnten aller Fehler unabhängig von der Verbindung möchten , machen Sie die logit-Klasse statisch anstelle eines Mitglieds.

Wie bereits erwähnt, sollte dies threadsicher sein:

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

Führen Sie im try-Block den folgenden Test durch:

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

Hinweis: Ich halte es nicht für eine gute Idee, 90% der Fehler zu beseitigen.

tpdi
quelle
1

Ich habe so etwas schon einmal gesehen.

Es gab einen Code, der bestimmte "Fragen" beantworten konnte, die von einem anderen "Black Box" -Code stammten. Falls es sie nicht beantworten konnte, leitete es sie an einen anderen Teil des Black-Box-Codes weiter, der sehr langsam war.

Manchmal tauchten also bisher ungesehene neue "Fragen" auf, und sie tauchten in einem Stapel auf, wie 100 von ihnen hintereinander.

Der Programmierer war mit der Funktionsweise des Programms zufrieden, wollte jedoch die Software in Zukunft verbessern, wenn möglich neue Fragen entdeckt wurden.

Die Lösung bestand also darin, unbekannte Fragen zu protokollieren, aber wie sich herausstellte, gab es Tausende verschiedener Fragen. Die Protokolle wurden zu groß, und es hatte keinen Vorteil, diese zu beschleunigen, da sie keine offensichtlichen Antworten hatten. Aber hin und wieder tauchten eine Reihe von Fragen auf, die beantwortet werden konnten.

Da die Protokolle zu groß wurden und die Protokollierung die Protokollierung der wirklich wichtigen Dinge behinderte, bekam er diese Lösung:

Protokollieren Sie nur zufällige 5%, um die Protokolle zu bereinigen, während auf lange Sicht immer noch angezeigt wird, welche Fragen / Antworten hinzugefügt werden könnten.

Wenn also ein unbekanntes Ereignis in einer zufälligen Anzahl dieser Fälle eintritt, wird es protokolliert.

Ich denke, das ist ähnlich wie das, was Sie hier sehen.

Diese Arbeitsweise hat mir nicht gefallen, deshalb habe ich diesen Code entfernt und diese Nachrichten einfach in einer anderen Datei protokolliert , sodass sie alle vorhanden waren, aber die allgemeine Protokolldatei nicht überlasteten.

Jens Timmerman
quelle
3
Abgesehen davon, dass es sich hier um einen Datenbanktreiber handelt ... falscher Problembereich, IMO!
Steven Schlansker
@StevenSchlansker Ich habe nie gesagt, dass dies eine gute Praxis ist. Ich habe diesen Code entfernt und diese Nachrichten einfach in einer anderen Datei protokolliert.
Jens Timmerman