Por que o driver MongoDB Java usa um gerador de números aleatórios de forma condicional?

211

Eu vi o código a seguir neste commit para o driver Java Connection do MongoDB e ele parece a princípio uma piada de algum tipo. O que o código a seguir faz?

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

(EDIT: o código foi atualizado desde a publicação desta pergunta)

Monstieur
fonte
13
Qual parte está confundindo você?
Oliver Charlesworth
4
Eu acho confuso. esse código é executado em um bloco catch!
Proviste
11
@MarkoTopolnik: É? Poderia ser escrito muito mais claramente como if (!ok || Math.random() < 0.1)(ou algo semelhante).
Oliver Charlesworth
5
github.com/mongodb/mongo-java-driver/commit/... você não está em primeiro lugar, ver comentário a essa linha
msangel
3
@msangel Esses caras parecem estar criticando a lógica, não o estilo de codificação.
Marko Topolnik

Respostas:

279

Depois de inspecionar o histórico dessa linha, minha principal conclusão é que houve alguma programação incompetente em ação.

  1. Essa linha é gratuita e complicada. A forma geral

    a? true : b

    pois boolean a, bé equivalente ao simples

    a || b
  2. A negação circundante e os parênteses excessivos envolvem ainda mais as coisas. Tendo em mente as leis de De Morgan , é uma observação trivial que esse trecho de código equivale a

    if (!_ok && Math.random() <= 0.1)
      return res;
  3. O commit que originalmente introduziu essa lógica tinha

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

    - outro exemplo de codificação incompetente, mas observe a lógica invertida : aqui o evento é registrado se _okou em 10% dos outros casos, enquanto o código em 2. retorna 10% das vezes e registra 90% das vezes. Assim, o cometimento posterior arruinou não apenas a clareza, mas também a correção.

    Acho que no código que você postou, podemos realmente ver como o autor pretendia transformar o original de if-thenalguma forma literalmente na negação necessária para a returncondição inicial . Mas então ele errou e inseriu um "duplo negativo" eficaz, invertendo o sinal de desigualdade.

  4. Excluindo os problemas de estilo de codificação, o registro estocástico é uma prática bastante dúbia por si só, especialmente porque a entrada do registro não documenta seu próprio comportamento peculiar. A intenção é, obviamente, reduzir as reformulações do mesmo fato: que o servidor esteja inativo no momento. A solução apropriada é registrar apenas alterações do estado do servidor, e não cada uma de suas observações, e muito menos uma seleção aleatória de 10% dessas observações. Sim, isso exige um pouco mais de esforço, então vamos ver alguns.

Só espero que toda essa evidência de incompetência, acumulada pela inspeção de apenas três linhas de código , não fale de maneira justa do projeto como um todo e que esse trabalho seja limpo o mais rápido possível.

Marko Topolnik
fonte
26
Além disso, isso parece ser, até onde eu sei, o driver Java 10gen oficial do MongoDB, portanto, além de ter uma opinião sobre o driver Java, acho que me dá uma opinião sobre o código do MongoDB
Chris Travers,
5
Excelente análise de apenas algumas linhas de código, posso transformá-lo em uma pergunta de entrevista! Seu quarto ponto é a verdadeira chave para a existência de algo fundamentalmente errado neste projeto (os outros podem ser descartados como erros infelizes do programador).
Abel
1
@ChrisTravers Ele é o piloto oficial mongo java para mongo.
assylias
17

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

Há 11 horas por gareth-rees:

Presumivelmente, a idéia é registrar apenas 1/10 das falhas do servidor (e, assim, evitar o envio massivo de spam), sem incorrer no custo de manutenção de um contador ou timer. (Mas certamente manter um cronômetro seria acessível?)

msangel
fonte
13
Não para escolher, mas: 1/10 das vezes retornará res, para registrar as outras 9/10 vezes.
Supericy
23
@ Supericy Isso definitivamente não é nada. Isso é apenas mais uma evidência das terríveis práticas de codificação dessa pessoa.
Anorov 31/05
7

Adicione um membro da classe inicializado ao negativo 1:

  private int logit = -1;

No bloco try, faça o teste:

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

Isso sempre registra o primeiro erro e, em seguida, cada décimo erro subsequente. Os operadores lógicos "curto-circuitam"; portanto, o logit é incrementado apenas com um erro real.

Se você deseja o primeiro e o décimo de todos os erros, independentemente da conexão, torne a classe logit estática em vez de um membro.

Como foi observado, isso deve ser seguro para threads:

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

No bloco try, faça o teste:

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

Nota: Não acho que jogar 90% dos erros seja uma boa ideia.

tpdi
fonte
1

Eu já vi esse tipo de coisa antes.

Havia um pedaço de código que poderia responder a certas "perguntas" que vinham de outro pedaço de código da "caixa preta". Caso não pudesse respondê-las, as encaminharia para outra parte do código da 'caixa preta' que era realmente lenta.

Então, algumas vezes, novas 'perguntas' nunca vistas antes apareciam, e elas apareciam em um lote, como 100 delas seguidas.

O programador ficou satisfeito com o funcionamento do programa, mas queria uma maneira de melhorar o software no futuro, se possíveis novas perguntas fossem descobertas.

Portanto, a solução foi registrar perguntas desconhecidas, mas, como se viu, havia milhares de perguntas diferentes. Os logs ficaram grandes demais e não havia nenhum benefício em acelerá-los, pois não tinham respostas óbvias. De vez em quando, porém, aparecia um lote de perguntas que poderiam ser respondidas.

Como os logs estavam ficando muito grandes e o registro estava impedindo o registro das coisas realmente importantes que ele conseguiu nessa solução:

Registre apenas 5% aleatórios; isso limpará os registros, enquanto, a longo prazo, ainda mostra quais perguntas / respostas podem ser adicionadas.

Portanto, se um evento desconhecido ocorresse, em uma quantidade aleatória desses casos, ele seria registrado.

Eu acho que isso é semelhante ao que você está vendo aqui.

Como não gostei dessa maneira de trabalhar, removi esse código e apenas registrei essas mensagens em um arquivo diferente , para que todos estivessem presentes, mas sem prejudicar o arquivo de log geral.

Jens Timmerman
fonte
3
Só que estamos falando de um driver de banco de dados aqui ... espaço errado do problema, IMO!
Steven Schlansker,
@StevenSchlansker Eu nunca disse que essa era uma boa prática. Eu removi este pedaço de código e apenas registrei essas mensagens em um arquivo diferente.
usar o seguinte