Dlaczego sterownik Java MongoDB korzysta z generatora liczb losowych w warunkach warunkowych?

211

Widziałem następujący kod w tym zatwierdzeniu dla sterownika Java Connection MongoDB i na początku wydaje się, że to jakiś żart. Co robi następujący kod?

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

(EDYCJA: kod został zaktualizowany od czasu opublikowania tego pytania)

Monstieur
źródło
13
Która część Cię dezorientuje?
Oliver Charlesworth
4
myślę, że to mylące. ten kod jest wykonywany w bloku catch!
Proviste
11
@MarkoTopolnik: Czy to jest? Można to zapisać znacznie jaśniej jako if (!ok || Math.random() < 0.1)(lub coś podobnego).
Oliver Charlesworth
5
github.com/mongodb/mongo-java-driver/commit/… nie jesteś pierwszy, patrz komentarz do tej linii
msangel
3
@msangel Ci faceci wydają się krytykować logikę, a nie styl kodowania.
Marko Topolnik

Odpowiedzi:

279

Po sprawdzeniu historii tej linii, mój główny wniosek jest taki, że w pracy działało pewne niekompetentne programowanie.

  1. Ta linia jest nieuzasadniona. Ogólna forma

    a? true : b

    bo boolean a, bjest równoważne prostemu

    a || b
  2. Otaczające zaprzeczenie i nadmierna liczba nawiasów jeszcze bardziej pogmatwają sprawy. Pamiętając o prawach De Morgana, spostrzeżenie to jest banalne

    if (!_ok && Math.random() <= 0.1)
      return res;
  3. Zatwierdzenie, które pierwotnie wprowadziło tę logikę, miało

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

    - inny przykład niekompetentnego kodowania, ale zwróć uwagę na odwrotną logikę : tutaj zdarzenie jest rejestrowane w jednym _oklub w 10% innych przypadków, podczas gdy kod w 2. zwraca 10% razy i loguje 90% razy. Późniejsze popełnienie zrujnowało nie tylko jasność, ale i samą poprawność.

    Myślę, że w opublikowanym kodzie możemy zobaczyć, w jaki sposób autor zamierzał przekształcić oryginał if-thendosłownie w negację wymaganą we wczesnym returnstanie. Ale potem pomieszał i wstawił skuteczny „podwójny negatyw”, odwracając znak nierówności.

  4. Pomijając kwestie związane ze stylem kodowania, rejestrowanie stochastyczne samo w sobie jest dość wątpliwą praktyką, zwłaszcza, że ​​wpis dziennika nie dokumentuje własnego osobliwego zachowania. Chodzi oczywiście o ograniczenie powtórzeń tego samego faktu: serwer nie działa. Właściwym rozwiązaniem jest rejestrowanie tylko zmian stanu serwera, a nie każdej jego obserwacji, nie mówiąc już o losowym wyborze 10% takich obserwacji. Tak, to wymaga trochę więcej wysiłku, więc zobaczmy trochę.

Mogę tylko mieć nadzieję, że wszystkie te dowody niekompetencji, zgromadzone podczas kontroli tylko trzech linii kodu , nie mówią uczciwie o całym projekcie i że ten kawałek pracy zostanie jak najszybciej oczyszczony.

Marko Topolnik
źródło
26
Dodatkowo wydaje się, że jest to, o ile mogę powiedzieć, oficjalny sterownik Java 10gen dla MongoDB, więc oprócz opinii na temat sterownika Java, myślę, że daje mi on opinię na temat kodu MongoDB
Chris Travers
5
Doskonała analiza zaledwie kilku wierszy kodu, mogę po prostu przekształcić go w pytanie do rozmowy kwalifikacyjnej! Twój czwarty punkt jest prawdziwym kluczem do tego, dlaczego coś jest zasadniczo nie tak z tym projektem (inne mogą zostać odrzucone jako niefortunne błędy programisty).
Abel
1
@ChrisTravers Jest to oficjalny sterownik Mongo Java dla Mongo.
assylias
17

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

11 godzin temu autor: gareth-rees:

Prawdopodobnie chodzi o rejestrowanie tylko około 1/10 awarii serwera (a więc uniknięcie masowego spamowania dziennika), bez ponoszenia kosztów utrzymania licznika lub timera. (Ale na pewno utrzymanie timera byłoby niedrogie?)

msangel
źródło
13
Nie, aby wybrać nitpick, ale: 1/10 razy zwróci res, więc będzie logował pozostałe 9/10 razy.
Supericy
23
@Supericy To zdecydowanie nie nitpicking. To tylko kolejny dowód na okropne praktyki kodowania tej osoby.
Anorov,
7

Dodaj członka klasy zainicjowanego do ujemnego 1:

  private int logit = -1;

W bloku try wykonaj test:

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

To zawsze rejestruje pierwszy błąd, a następnie co dziesiąty kolejny błąd. Operatory logiczne „zwarły”, więc logit zwiększa się tylko w przypadku rzeczywistego błędu.

Jeśli chcesz mieć pierwszy i dziesiąty ze wszystkich błędów, niezależnie od połączenia, ustaw statyczną klasę logit zamiast elementu członkowskiego.

Jak już wspomniano, powinno to być bezpieczne dla wątków:

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

W bloku try wykonaj test:

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

Uwaga: Nie sądzę, aby wyrzucenie 90% błędów było dobrym pomysłem.

tpdi
źródło
1

Widziałem już takie rzeczy wcześniej.

Był fragment kodu, który mógł odpowiedzieć na niektóre „pytania” pochodzące z innego fragmentu kodu „czarnej skrzynki”. W przypadku, gdy nie mógł na nie odpowiedzieć, przekierowałby ich do innego fragmentu „czarnej skrzynki”, który był naprawdę wolny.

Czasami więc pojawiały się nowe, niewidoczne wcześniej „pytania”, które pojawiały się w partii, jak 100 z nich z rzędu.

Programista był zadowolony z tego, jak działał program, ale chciał w jakiś sposób ulepszyć oprogramowanie w przyszłości, jeśli to możliwe, odkryć nowe pytania.

Rozwiązaniem było rejestrowanie nieznanych pytań, ale jak się okazało, było ich tysiące. Kłody stały się zbyt duże i przyspieszenie ich nie było korzystne, ponieważ nie mieli oczywistych odpowiedzi. Ale co jakiś czas pojawiała się seria pytań, na które można było odpowiedzieć.

Ponieważ dzienniki stawały się zbyt duże, a rejestrowanie utrudniało rejestrowanie naprawdę ważnych rzeczy, które dostał do tego rozwiązania:

Loguj tylko losowo 5%, to wyczyści logi, a na dłuższą metę nadal pokaże, jakie pytania / odpowiedzi można dodać.

Tak więc, jeśli wystąpi nieznane zdarzenie, w losowej liczbie tych przypadków zostanie zarejestrowane.

Myślę, że jest to podobne do tego, co tu widzisz.

Nie podobał mi się ten sposób pracy, więc usunąłem ten fragment kodu i po prostu zapisałem te wiadomości w innym pliku , więc wszystkie były obecne, ale nie blokowały ogólnego pliku dziennika.

Jens Timmerman
źródło
3
Tyle że tutaj mówimy o sterowniku bazy danych ... niewłaściwa przestrzeń problemu, IMO!
Steven Schlansker
@StevenSchlansker Nigdy nie mówiłem, że to dobra praktyka. Usunąłem ten fragment kodu i po prostu zapisałem te wiadomości w innym pliku.
Jens Timmerman