Geekly articles weekly

Qualidade de código do Apache Hadoop: teste de produção vs.

Figura 1

Para obter um código de produção de alta qualidade, não basta garantir a máxima cobertura com os testes. Sem dúvida, ótimos resultados exigem que o código principal do projeto e os testes funcionem eficientemente juntos. Portanto, os testes devem receber tanta atenção quanto o código principal. Um teste decente é um fator chave para o sucesso, uma vez que sofrerá regressão na produção. Vamos dar uma olhada nos avisos do analisador estático do PVS-Studio para ver a importância do fato de que os erros nos testes não são piores que os da produção. Foco de hoje: Apache Hadoop.

Sobre o projeto


Aqueles que anteriormente estavam interessados ​​em Big Data provavelmente já ouviram ou até trabalharam com o projeto Apache Hadoop . Em poucas palavras, o Hadoop é uma estrutura que pode ser usada como base para criar sistemas de Big Data e trabalhar com eles.

O Hadoop consiste em quatro módulos principais, cada um deles executa uma tarefa específica necessária para um sistema de análise de big data:

  • Hadoop comum
  • Mapreduce
  • Sistema de arquivos distribuídos do Hadoop
  • Fios

De qualquer forma, há muitos materiais sobre isso na Internet.

Sobre a verificação


Conforme mostrado na documentação, o PVS-Studio pode ser integrado ao projeto de várias maneiras:

  • Usando o plugin maven;
  • Usando o plugin gradle;
  • Usando o gradle IntellJ IDEA;
  • Diretamente usando o analisador.

O Hadoop é baseado no sistema de criação de maven, portanto, não houve obstáculos na verificação.

Depois de integrar o script da documentação e editar um dos arquivos pom.xml (havia módulos nas dependências que não estavam disponíveis), a análise começou!

Após a conclusão da análise, escolhi os avisos mais interessantes e notei que tinha o mesmo número de avisos no código de produção e nos testes. Normalmente, não considero avisos do analisador, dados para testes. Mas quando os dividi, não pude deixar as advertências de 'testes' desacompanhadas. “Por que não dar uma olhada neles?” - pensei, porque erros nos testes também podem ter consequências adversas. Eles podem levar a testes incorretos ou parciais, ou até mesmo a erros (eles existem apenas para marcar a caixa, se estão sempre verdes).

Depois de selecionar os avisos mais intrigantes, os dividi pelos seguintes grupos: produção, teste e os quatro principais módulos do Hadoop. E agora, fico feliz em oferecer a sua atenção a revisão dos avisos do analisador.

Código de produção


Hadoop comum


V6033 Um item com a mesma chave 'KDC_BIND_ADDRESS' já foi adicionado. MiniKdc.java (163), MiniKdc.java (162)

public class MiniKdc { .... private static final Set<String> PROPERTIES = new HashSet<String>(); .... static { PROPERTIES.add(ORG_NAME); PROPERTIES.add(ORG_DOMAIN); PROPERTIES.add(KDC_BIND_ADDRESS); PROPERTIES.add(KDC_BIND_ADDRESS); // <= PROPERTIES.add(KDC_PORT); PROPERTIES.add(INSTANCE); .... } .... }

O valor adicionado duas vezes no HashSet é um defeito muito comum na verificação de projetos. A segunda adição será realmente ignorada. Espero que essa duplicação seja apenas uma tragédia desnecessária. E se outro valor fosse criado para ser adicionado?

Mapreduce


V6072 Dois fragmentos de código semelhantes foram encontrados. Talvez seja um erro de digitação e a variável 'localFiles' deva ser usada em vez de 'localArchives'. LocalDistributedCacheManager.java (183), LocalDistributedCacheManager.java (178), LocalDistributedCacheManager.java (176), LocalDistributedCacheManager.java (181)

 public synchronized void setup(JobConf conf, JobID jobId) throws IOException { .... // Update the configuration object with localized data. if (!localArchives.isEmpty()) { conf.set(MRJobConfig.CACHE_LOCALARCHIVES, StringUtils .arrayToString(localArchives.toArray(new String[localArchives // <= .size()]))); } if (!localFiles.isEmpty()) { conf.set(MRJobConfig.CACHE_LOCALFILES, StringUtils .arrayToString(localFiles.toArray(new String[localArchives // <= .size()]))); } .... }

O diagnóstico V6072 às vezes produz algumas descobertas interessantes. O objetivo deste diagnóstico é detectar os mesmos fragmentos de código de tipo resultantes da cópia-pasta e substituição de uma-duas variáveis. Nesse caso, algumas variáveis ​​foram deixadas "não alteradas".

O código acima demonstra isso. No primeiro bloco, a variável localArchives é usada, no segundo fragmento semelhante - localFiles . Se você estudar esse código com a devida diligência, em vez de executá-lo rapidamente, como geralmente acontece durante a revisão do código, você notará o fragmento, onde o autor esqueceu de substituir a variável localArchives .

Essa gafe pode levar ao seguinte cenário:

  • Suponha que tenhamos arquivos locais (tamanho = 4) e arquivos locais (tamanho = 2);
  • Ao criar a matriz localFiles.toArray (nova String [localArchives.size ()]) , os dois últimos elementos serão nulos (["pathToFile1", "pathToFile2", null, null]);
  • Então org.apache.hadoop.util.StringUtils.arrayToString retornará a representação de string da nossa matriz, na qual os nomes dos últimos arquivos serão apresentados como "nulos" ("pathToFile1, pathToFile2, null, null" ) ;
  • Tudo isso será passado adiante e Deus sabe apenas que tipo de verificação existem para esses casos =).

A expressão V6007 'children.size ()> 0' sempre é verdadeira. Queue.java (347)

 boolean isHierarchySameAs(Queue newState) { .... if (children == null || children.size() == 0) { .... } else if(children.size() > 0) { .... } .... }

Devido ao fato de o número de elementos ser verificado como 0 separadamente, a verificação adicional children.size ()> 0 sempre será verdadeira.

HDFS


V6001 Existem subexpressões idênticas 'this.bucketSize' à esquerda e à direita do operador '%'. RollingWindow.java (79)

  RollingWindow(int windowLenMs, int numBuckets) { buckets = new Bucket[numBuckets]; for (int i = 0; i < numBuckets; i++) { buckets[i] = new Bucket(); } this.windowLenMs = windowLenMs; this.bucketSize = windowLenMs / numBuckets; if (this.bucketSize % bucketSize != 0) { // <= throw new IllegalArgumentException( "The bucket size in the rolling window is not integer: windowLenMs= " + windowLenMs + " numBuckets= " + numBuckets); } }

Aqui o defeito é que a variável é dividida por ela mesma. Como resultado, a verificação da multiplicidade sempre será bem-sucedida e, no caso de obter entradas incorretas ( windowLenMs , numBuckets ), a exceção nunca será lançada.

Fios


V6067 Dois ou mais ramificações de caso executam as mesmas ações. TimelineEntityV2Converter.java (386), TimelineEntityV2Converter.java (389)

  public static ApplicationReport convertToApplicationReport(TimelineEntity entity) { .... if (metrics != null) { long vcoreSeconds = 0; long memorySeconds = 0; long preemptedVcoreSeconds = 0; long preemptedMemorySeconds = 0; for (TimelineMetric metric : metrics) { switch (metric.getId()) { case ApplicationMetricsConstants.APP_CPU_METRICS: vcoreSeconds = getAverageValue(metric.getValues().values()); break; case ApplicationMetricsConstants.APP_MEM_METRICS: memorySeconds = ....; break; case ApplicationMetricsConstants.APP_MEM_PREEMPT_METRICS: preemptedVcoreSeconds = ....; // <= break; case ApplicationMetricsConstants.APP_CPU_PREEMPT_METRICS: preemptedVcoreSeconds = ....; // <= break; default: // Should not happen.. break; } } .... } .... }

Os mesmos fragmentos de código em dois ramos do caso . Está em todo lugar! No número prevalecente de casos, esse não é um erro real, mas apenas um motivo para pensar sobre o fato de alterar a opção . Mas não para o caso em questão. Fragmentos de código repetidos configuram o valor da variável preemptedVcoreSeconds . Se você observar atentamente os nomes de todas as variáveis ​​e constantes, provavelmente concluirá que, se metric.getId () == APP_MEM_PREEMPT_METRICS, o valor deve ser definido para a variável preemptedMemorySeconds , não para preemptedVcoreSeconds . Nesse sentido, após o operador 'switch', preemptedMemorySeconds sempre permanecerá 0, enquanto o valor de preemptedVcoreSeconds pode estar incorreto.

V6046 Formato incorreto. É esperado um número diferente de itens de formato. Argumentos não utilizados: 2. AbstractSchedulerPlanFollower.java (186)

 @Override public synchronized void synchronizePlan(Plan plan, boolean shouldReplan) { .... try { setQueueEntitlement(planQueueName, ....); } catch (YarnException e) { LOG.warn("Exception while trying to size reservation for plan: {}", currResId, planQueueName, e); } .... }

A variável planQueueName não é usada durante o log. Nesse caso, muita coisa é copiada ou a sequência de formatação não está concluída. Mas ainda culpo o bom e velho copiar-colar, que em alguns casos é ótimo para dar um tiro no pé.

Código de teste


Hadoop comum


V6072 Dois fragmentos de código semelhantes foram encontrados. Talvez seja um erro de digitação e a variável 'allSecretsB' deva ser usada em vez de 'allSecretsA'. TestZKSignerSecretProvider.java (316), TestZKSignerSecretProvider.java (309), TestZKSignerSecretProvider.java (306), TestZKSignerSecretProvider.java (313)

 public void testMultiple(int order) throws Exception { .... currentSecretA = secretProviderA.getCurrentSecret(); allSecretsA = secretProviderA.getAllSecrets(); Assert.assertArrayEquals(secretA2, currentSecretA); Assert.assertEquals(2, allSecretsA.length); // <= Assert.assertArrayEquals(secretA2, allSecretsA[0]); Assert.assertArrayEquals(secretA1, allSecretsA[1]); currentSecretB = secretProviderB.getCurrentSecret(); allSecretsB = secretProviderB.getAllSecrets(); Assert.assertArrayEquals(secretA2, currentSecretB); Assert.assertEquals(2, allSecretsA.length); // <= Assert.assertArrayEquals(secretA2, allSecretsB[0]); Assert.assertArrayEquals(secretA1, allSecretsB[1]); .... }

E novamente o V6072. Observe atentamente as variáveis allSecretsA e allSecretsB .

V6043 Considere inspecionar o operador 'for'. Os valores inicial e final do iterador são os mesmos. TestTFile.java (235)

 private int readPrepWithUnknownLength(Scanner scanner, int start, int n) throws IOException { for (int i = start; i < start; i++) { String key = String.format(localFormatter, i); byte[] read = readKey(scanner); assertTrue("keys not equal", Arrays.equals(key.getBytes(), read)); try { read = readValue(scanner); assertTrue(false); } catch (IOException ie) { // should have thrown exception } String value = "value" + key; read = readLongValue(scanner, value.getBytes().length); assertTrue("values nto equal", Arrays.equals(read, value.getBytes())); scanner.advance(); } return (start + n); }

Um teste sempre verde? =). Uma parte do loop, que faz parte do próprio teste, nunca será executada. Isso se deve ao fato de que os valores inicial e final do contador são iguais na instrução for . Como resultado, a condição i <start se tornará imediatamente falsa, levando a esse comportamento. Corri o arquivo de teste e cheguei à conclusão de que, na verdade, eu <(start + n) tinha que ser escrito na condição de loop.

Mapreduce


A expressão V6007 'byteAm <0' é sempre falsa. DataWriter.java (322)

 GenerateOutput writeSegment(long byteAm, OutputStream out) throws IOException { long headerLen = getHeaderLength(); if (byteAm < headerLen) { // not enough bytes to write even the header return new GenerateOutput(0, 0); } // adjust for header length byteAm -= headerLen; if (byteAm < 0) { // <= byteAm = 0; } .... }

A condição byteAm <0 é sempre falsa. Para descobrir, vamos dar uma olhada no código acima. Se a execução do teste atingir a operação byteAm - = headerLen , significa que byteAm> = headerLen. A partir daqui, após a subtração, o valor byteAm nunca será negativo. Isso é o que tivemos que provar.

HDFS


V6072 Dois fragmentos de código semelhantes foram encontrados. Talvez seja um erro de digitação e a variável 'normalFile' deva ser usada em vez de 'normalDir'. TestWebHDFS.java (625), TestWebHDFS.java (615), TestWebHDFS.java (614), TestWebHDFS.java (624)

 public void testWebHdfsErasureCodingFiles() throws Exception { .... final Path normalDir = new Path("/dir"); dfs.mkdirs(normalDir); final Path normalFile = new Path(normalDir, "file.log"); .... // logic block #1 FileStatus expectedNormalDirStatus = dfs.getFileStatus(normalDir); FileStatus actualNormalDirStatus = webHdfs.getFileStatus(normalDir); // <= Assert.assertEquals(expectedNormalDirStatus.isErasureCoded(), actualNormalDirStatus.isErasureCoded()); ContractTestUtils.assertNotErasureCoded(dfs, normalDir); assertTrue(normalDir + " should have erasure coding unset in " + ....); // logic block #2 FileStatus expectedNormalFileStatus = dfs.getFileStatus(normalFile); FileStatus actualNormalFileStatus = webHdfs.getFileStatus(normalDir); // <= Assert.assertEquals(expectedNormalFileStatus.isErasureCoded(), actualNormalFileStatus.isErasureCoded()); ContractTestUtils.assertNotErasureCoded(dfs, normalFile); assertTrue( normalFile + " should have erasure coding unset in " + ....); }

Acredite ou não, é V6072 novamente! Basta seguir as variáveis normalDir e normalFile.

V6027 As variáveis ​​são inicializadas através da chamada para a mesma função. Provavelmente é um erro ou código não otimizado. TestDFSAdmin.java (883), TestDFSAdmin.java (879)

 private void verifyNodesAndCorruptBlocks( final int numDn, final int numLiveDn, final int numCorruptBlocks, final int numCorruptECBlockGroups, final DFSClient client, final Long highestPriorityLowRedundancyReplicatedBlocks, final Long highestPriorityLowRedundancyECBlocks) throws IOException { /* init vars */ .... final String expectedCorruptedECBlockGroupsStr = String.format( "Block groups with corrupt internal blocks: %d", numCorruptECBlockGroups); final String highestPriorityLowRedundancyReplicatedBlocksStr = String.format( "\tLow redundancy blocks with highest priority " + "to recover: %d", highestPriorityLowRedundancyReplicatedBlocks); final String highestPriorityLowRedundancyECBlocksStr = String.format( "\tLow redundancy blocks with highest priority " + "to recover: %d", highestPriorityLowRedundancyReplicatedBlocks); .... }

Nesse fragmento, as variáveis maximumPriorityLowRedundancyReplicatedBlocksStr e maximumPriorityLowRedundancyECBlocksStr são inicializadas pelos mesmos valores. Muitas vezes deve ser assim, mas este não é o caso. Os nomes das variáveis ​​são longos e similares, portanto, não é de surpreender que o fragmento copiado e colado não tenha sido alterado de forma alguma. Para corrigi-lo, ao inicializar a variável maximumPriorityLowRedundancyECBlocksStr , o autor deve usar o parâmetro de entrada maximumPriorityLowRedundancyECBlocks . Além disso, provavelmente, eles ainda precisam corrigir a linha de formato.

V6019 Código inacessível detectado. É possível que haja um erro. TestReplaceDatanodeFailureReplication.java (222)

 private void verifyFileContent(...., SlowWriter[] slowwriters) throws IOException { LOG.info("Verify the file"); for (int i = 0; i < slowwriters.length; i++) { LOG.info(slowwriters[i].filepath + ....); FSDataInputStream in = null; try { in = fs.open(slowwriters[i].filepath); for (int j = 0, x;; j++) { x = in.read(); if ((x) != -1) { Assert.assertEquals(j, x); } else { return; } } } finally { IOUtils.closeStream(in); } } }

O analisador reclama que o contador i ++ no loop não pode ser alterado. O que significa que no loop for (int i = 0; i <slowwriters.length; i ++) {....}, não mais do que uma iteração será executada. Vamos descobrir o porquê. Portanto, na primeira iteração, vinculamos o thread ao arquivo que corresponde a slowwriters [0] para leitura adicional. Em seguida, lemos o conteúdo do arquivo através do loop for (int j = 0, x ;; j ++):

  • se lemos algo relevante, comparamos o byte lido com o valor atual do contador j, embora assertEquals (se a verificação não for bem-sucedida, falhamos no teste);
  • se o arquivo for verificado com êxito e chegarmos ao final do arquivo (lemos -1), o método será encerrado.

Portanto, o que quer que aconteça durante a verificação de slowwriters [0] , não será possível verificar os elementos subsequentes. Provavelmente, é necessário usar o intervalo em vez do retorno.

Fios


V6019 Código inacessível detectado. É possível que haja um erro. TestNodeManager.java (176)

 @Test public void testCreationOfNodeLabelsProviderService() throws InterruptedException { try { .... } catch (Exception e) { Assert.fail("Exception caught"); e.printStackTrace(); } }

Nessa situação, o método Assert.fail interromperá o teste e o rastreamento de pilha não será impresso em caso de exceção. Se a mensagem sobre a exceção capturada for suficiente aqui, é melhor remover a impressão do stacktrace para evitar confusão. Se for necessário imprimir, basta trocá-los.

Muitos fragmentos semelhantes foram encontrados:

  • V6019 Código inacessível detectado. É possível que haja um erro. TestResourceTrackerService.java (928)
  • V6019 Código inacessível detectado. É possível que haja um erro. TestResourceTrackerService.java (737)
  • V6019 Código inacessível detectado. É possível que haja um erro. TestResourceTrackerService.java (685)
  • ....

V6072 Dois fragmentos de código semelhantes foram encontrados. Talvez seja um erro de digitação e a variável 'publicCache' deva ser usada em vez de 'usercache'. TestResourceLocalizationService.java (315), TestResourceLocalizationService.java (309), TestResourceLocalizationService.java (307), TestResourceLocalizationService.java (317)

 @Test public void testDirectoryCleanupOnNewlyCreatedStateStore() throws IOException, URISyntaxException { .... // verify directory creation for (Path p : localDirs) { p = new Path((new URI(p.toString())).getPath()); // logic block #1 Path usercache = new Path(p, ContainerLocalizer.USERCACHE); verify(spylfs).rename(eq(usercache), any(Path.class), any()); // <= verify(spylfs).mkdir(eq(usercache), ....); // logic block #2 Path publicCache = new Path(p, ContainerLocalizer.FILECACHE); verify(spylfs).rename(eq(usercache), any(Path.class), any()); // <= verify(spylfs).mkdir(eq(publicCache), ....); .... } .... }

E, finalmente, V6072 novamente =). Variáveis ​​para visualizar o fragmento suspeito: usercache e publicCache .

Conclusão


Centenas de milhares de linhas de código são escritas em desenvolvimento. O código de produção geralmente é mantido livre de bugs, defeitos e falhas (os desenvolvedores testam seu código, o código é revisado e assim por diante). Os testes são definitivamente inferiores a esse respeito. Defeitos nos testes podem se esconder facilmente atrás de um "sinal verde". Como você provavelmente recebeu da revisão de avisos de hoje, um teste verde nem sempre é uma verificação bem-sucedida.

Dessa vez, ao verificar a base de código do Apache Hadoop, a análise estática acabou sendo altamente necessária no código de produção e nos testes que também desempenham um papel importante no desenvolvimento.

Portanto, se você se importa com o seu código e testa a qualidade, sugiro que você analise a estática. Deixe o PVS-Studio ser o primeiro candidato neste empreendimento.

Source: https://habr.com/ru/post/pt480918/

More articles:


All Articles