Geekly articles weekly

Apache Hadoop Code Quality: Production VS Test

Figura 1

Para obtener un código de producción de alta calidad, no basta con garantizar la máxima cobertura con las pruebas. Sin dudas, los excelentes resultados requieren que el código principal del proyecto y las pruebas funcionen juntos de manera eficiente. Por lo tanto, las pruebas deben prestarse tanta atención como el código principal. Una prueba decente es un factor clave de éxito, ya que detectará una regresión en la producción. Echemos un vistazo a las advertencias del analizador estático PVS-Studio para ver la importancia del hecho de que los errores en las pruebas no son peores que los de producción. El enfoque de hoy: Apache Hadoop.

Sobre el proyecto


Los que antes estaban interesados ​​en Big Data probablemente hayan escuchado o incluso trabajado con el proyecto Apache Hadoop . En pocas palabras, Hadoop es un marco que se puede utilizar como base para construir sistemas de Big Data y trabajar con ellos.

Hadoop consta de cuatro módulos principales, cada uno de ellos realiza una tarea específica requerida para un sistema de análisis de big data:

  • Hadoop común
  • Mapreduce
  • Sistema de archivos distribuidos de Hadoop
  • Hilo

De todos modos, hay muchos materiales al respecto en Internet.

Sobre el cheque


Como se muestra en la documentación, PVS-Studio se puede integrar en el proyecto de varias maneras:

  • Usando el complemento maven;
  • Usando el complemento gradle;
  • Usando la gradle IntellJ IDEA;
  • Usando directamente el analizador.

Hadoop se basa en el sistema de construcción maven, por lo tanto, no hubo obstáculos con el cheque.

Después de integrar el script de la documentación y editar uno de los archivos pom.xml (había módulos en dependencias que no estaban disponibles), ¡comenzó el análisis!

Después de completar el análisis, elegí las advertencias más interesantes y noté que tenía la misma cantidad de advertencias en el código de producción y en las pruebas. Normalmente, no considero las advertencias del analizador, dadas para las pruebas. Pero cuando los dividí, no pude dejar desatendidas las advertencias de 'pruebas'. "¿Por qué no echarles un vistazo?", Pensé, porque los errores en las pruebas también podrían tener consecuencias adversas. Pueden conducir a pruebas incorrectas o parciales, o incluso a mezclilla (existen solo para marcar la casilla, que siempre son verdes).

Después de seleccionar las advertencias más interesantes, las dividí entre los siguientes grupos: producción, prueba y los cuatro módulos principales de Hadoop. Y ahora me complace ofrecerle su atención la revisión de las advertencias del analizador.

Código de producción


Hadoop común


V6033 Ya se ha agregado un elemento con la misma clave 'KDC_BIND_ADDRESS'. 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); .... } .... }

El valor agregado dos veces en HashSet es un defecto muy común al verificar proyectos. La segunda adición será realmente ignorada. Espero que esta duplicación sea solo una tragedia innecesaria. ¿Qué pasa si se pretende agregar otro valor?

Mapreduce


V6072 Se encontraron dos fragmentos de código similares. Quizás, este es un error tipográfico y se debe usar la variable 'localFiles' en lugar 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()]))); } .... }

El diagnóstico V6072 a veces arroja algunos hallazgos interesantes. El propósito de este diagnóstico es detectar los mismos fragmentos de código de tipo resultado de copiar y pegar y el reemplazo de una o dos variables. En este caso, algunas variables incluso se han dejado "sin cambios".

El código anterior lo demuestra. En el primer bloque se usa la variable localArchives , en el segundo fragmento similar: localFiles . Si estudia este código con la debida diligencia, en lugar de ejecutarlo rápidamente, como sucede a menudo durante la revisión del código, notará el fragmento, donde el autor olvidó reemplazar la variable localArchives .

Este error puede conducir al siguiente escenario:

  • Supongamos que tenemos localArchives (size = 4) y localFiles (size = 2);
  • Al crear la matriz localFiles.toArray (new String [localArchives.size ()]) , los últimos 2 elementos serán nulos (["pathToFile1", "pathToFile2", null, null]);
  • Luego org.apache.hadoop.util.StringUtils.arrayToString devolverá la representación de cadena de nuestra matriz, en la que los últimos nombres de archivos se presentarán como "nulo" ("pathToFile1, pathToFile2, null, null" ) ;
  • Todo esto se pasará más allá y solo Dios sabe qué tipo de controles hay para tales casos =).

V6007 La expresión 'children.size ()> 0' siempre es verdadera. Queue.java (347)

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

Debido al hecho de que la cantidad de elementos se verifica para 0 por separado, la verificación adicional children.size ()> 0 siempre será verdadera.

HDFS


V6001 Hay subexpresiones idénticas 'this.bucketSize' a la izquierda y a la derecha del 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); } }

Aquí el defecto es que la variable se divide por sí misma. Como resultado, la verificación de multiplicidad siempre será exitosa y en caso de obtener entradas incorrectas ( windowLenMs , numBuckets ), la excepción nunca se lanzará.

Hilo


V6067 Dos o más ramificaciones de casos realizan las mismas acciones. 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; } } .... } .... }

Los mismos fragmentos de código en dos ramas de casos . ¡Está por todas partes! En el número predominante de casos, esto no es un error real, sino solo una razón para pensar en la refactorización del interruptor . Pero no para el caso en cuestión. Los fragmentos de código repetidos establecen el valor de la variable preemptedVcoreSeconds . Si observa detenidamente los nombres de todas las variables y constantes, probablemente concluirá que, en caso de que metric.getId () == APP_MEM_PREEMPT_METRICS, el valor debe establecerse para la variable preemptedMemorySeconds , no para preemptedVcoreSeconds . En este sentido, después del operador 'switch', preemptedMemorySeconds siempre permanecerá 0, mientras que el valor de preemptedVcoreSeconds podría ser incorrecto.

V6046 Formato incorrecto. Se espera un número diferente de elementos de formato. Argumentos no 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); } .... }

La variable planQueueName no se usa al iniciar sesión. En este caso, se copia demasiado o la cadena de formato no está terminada. Pero todavía culpo a la vieja copia y pegar, que en algunos casos es genial para dispararte en el pie.

Código de prueba


Hadoop común


V6072 Se encontraron dos fragmentos de código similares. Quizás, este es un error tipográfico y se debe usar la variable 'allSecretsB' en lugar 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]); .... }

Y de nuevo el V6072. Mire detenidamente las variables allSecretsA y allSecretsB .

V6043 Considere inspeccionar el operador 'para'. Los valores iniciales y finales del iterador son los mismos. 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); }

¿Una prueba que siempre es verde? =). Una parte del bucle, que es parte de la prueba en sí, nunca se ejecutará. Esto se debe al hecho de que los valores de contador inicial y final son iguales en la instrucción for . Como resultado, la condición i <start se convertirá inmediatamente en falsa, lo que conducirá a dicho comportamiento. Revisé el archivo de prueba y llegué a la conclusión de que en realidad i <(inicio + n) tenía que escribirse en la condición de bucle.

Mapreduce


V6007 La expresión 'byteAm <0' siempre es 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; } .... }

La condición byteAm <0 siempre es falsa. Para resolverlo, demos otro vistazo al código de arriba. Si la ejecución de la prueba alcanza la operación byteAm - = headerLen , significa que byteAm> = headerLen. Desde aquí, después de la resta, el valor byteAm nunca será negativo. Eso es lo que tuvimos que demostrar.

HDFS


V6072 Se encontraron dos fragmentos de código similares. Quizás, este es un error tipográfico y se debe usar la variable 'normalFile' en lugar 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 " + ....); }

Lo creas o no, ¡es V6072 nuevamente! Simplemente siga las variables normalDir y normalFile.

V6027 Las variables se inicializan a través de la llamada a la misma función. Probablemente sea un error o un código no optimizado. 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); .... }

En este fragmento, las variables más altasPriorityLowRedundancyReplicatedBlocksStr y más altasPriorityLowRedundancyECBlocksStr se inicializan con los mismos valores. A menudo debería ser así, pero este no es el caso. Los nombres de las variables son largos y similares, por lo que no es sorprendente que el fragmento copiado no haya cambiado de ninguna manera. Para solucionarlo, al inicializar la variable más altaPriorityLowRedundancyECBlocksStr , el autor tiene que usar el parámetro de entrada más altoPriorityLowRedundancyECBlocks . Además, lo más probable es que todavía necesiten corregir la línea de formato.

V6019 Código inalcanzable detectado. Es posible que haya un error presente. 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); } } }

El analizador se queja de que el contador i ++ en el bucle no se puede cambiar. Lo que significa que en el ciclo for (int i = 0; i <slowwriters.length; i ++) {....} no se ejecutará más de una iteración. Veamos por qué. Entonces, en la primera iteración, vinculamos el hilo con el archivo que corresponde a los escritores lentos [0] para leer más. A continuación, leemos el contenido del archivo a través del bucle para (int j = 0, x ;; j ++):

  • si leemos algo relevante, comparamos el byte de lectura con el valor actual del contador j, aunque afirmar Equal (si la verificación no es exitosa, fallamos en la prueba);
  • si el archivo se verifica con éxito y llegamos al final del archivo (leemos -1), el método se cierra.

Por lo tanto, pase lo que pase durante la comprobación de los escritores lentos [0] , no se comprobarán los elementos posteriores. Lo más probable es que se deba usar el descanso en lugar del retorno.

Hilo


V6019 Código inalcanzable detectado. Es posible que haya un error presente. TestNodeManager.java (176)

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

En esta situación, el método Assert.fail interrumpirá la prueba y stacktrace no se imprimirá en caso de una excepción. Si el mensaje sobre la excepción capturada es suficiente aquí, es mejor eliminar la impresión de stacktrace para evitar confusiones. Si es necesario imprimir, solo necesita intercambiarlos.

Se han encontrado muchos fragmentos similares:

  • V6019 Código inalcanzable detectado. Es posible que haya un error presente. TestResourceTrackerService.java (928)
  • V6019 Código inalcanzable detectado. Es posible que haya un error presente. TestResourceTrackerService.java (737)
  • V6019 Código inalcanzable detectado. Es posible que haya un error presente. TestResourceTrackerService.java (685)
  • ....

V6072 Se encontraron dos fragmentos de código similares. Quizás, este es un error tipográfico y se debe usar la variable 'publicCache' en lugar de 'usercache'. TestResourceLocalizationService.java (315), TestResourceLocalizationService.java (309), TestResourceLocalizationService.java (307), TestResourceLocalizationService.java (313)

 @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), ....); .... } .... }

Y finalmente, V6072 nuevamente =). Variables para ver el fragmento sospechoso: usercache y publicCache .

Conclusión


Cientos de miles de líneas de código están escritas en desarrollo. El código de producción generalmente se mantiene limpio de errores, defectos y fallas (los desarrolladores prueban su código, el código se revisa, etc.). Las pruebas son definitivamente inferiores en este sentido. Los defectos en las pruebas pueden esconderse fácilmente detrás de una "marca verde". Como probablemente haya obtenido de la revisión de advertencias de hoy, una prueba verde no siempre es una verificación exitosa.

Esta vez, al verificar la base de código de Apache Hadoop, el análisis estático resultó ser muy necesario tanto en el código de producción como en las pruebas que también juegan un papel importante en el desarrollo.

Entonces, si le importa su código y la calidad de las pruebas, le sugiero que fije su vista en el análisis estático. Deje que PVS-Studio sea ​​el primer competidor en esta empresa.

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

More articles:


All Articles