Geekly articles weekly

Apache Hadoop Code Quality: prueba de producción VS

Figura 1

Para obtener un código de producción de alta calidad, no es suficiente proporcionar la máxima cobertura de prueba. Sin lugar a dudas, para lograr altos resultados, el código principal del proyecto y las pruebas deben funcionar en un tándem perfectamente coherente. Por lo tanto, debe prestar atención a las pruebas tanto como al código principal. Escribir una buena prueba es la clave para detectar una regresión en la producción. Para mostrar la importancia del hecho de que los errores en las pruebas no son peores que en la producción, consideraremos el próximo análisis de advertencias del analizador estático PVS-Studio. Objetivo: Apache Hadoop.

Sobre el proyecto


Los que alguna vez estuvieron interesados ​​en Big Data probablemente hayan escuchado o trabajado con un proyecto como Apache Hadoop . En resumen, Hadoop es un marco que se puede utilizar como base para construir y trabajar con sistemas Big Data.

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

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

Sin embargo, hay muchos materiales para familiarizarse con él en Internet.

Acerca de Verificación


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 IntellJ IDEA
  • usando el analizador directamente.

Hadoop fue construido sobre la base del sistema de construcción maven, por lo que no hubo dificultades con la verificación.

Habiendo integrado el script de la documentación y corregido ligeramente uno de pom.xml (había módulos en las dependencias que no estaban allí), ¡el análisis fue!

Después del análisis, al elegir las advertencias más interesantes, noté que tenía la misma cantidad de advertencias tanto en el código de producción como en las pruebas. Por lo general, no considero la activación del analizador que cae en las pruebas. Pero, dividiéndolos, no podía pasar por alto las advertencias de la categoría 'pruebas'. "¿Por qué no?", Pensé, porque los errores en las pruebas también tienen consecuencias. Pueden conducir a pruebas incorrectas o parciales, o incluso a tonterías (solo para mostrar, de modo que siempre estén verdes).

Entonces, habiendo recopilado las advertencias más interesantes, dividiéndolas por código (producción, prueba) y los cuatro módulos principales de Hadoop, les traigo un análisis de las operaciones 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); .... } .... }

Un valor agregado dos veces a un HashSet es un defecto común al verificar proyectos. De hecho, la segunda adición será ignorada. Bueno, si esta duplicación es un accidente absurdo. ¿Pero qué pasa si realmente significa 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()]))); } .... }

Diagnostics V6072 a veces hace hallazgos muy interesantes. La esencia del diagnóstico es buscar el mismo tipo de fragmentos de código que se obtuvieron copiando y pegando y reemplazando una o dos variables, pero al mismo tiempo algunas variables fueron "subestimadas".

El código anterior lo demuestra. En el primer bloque, las acciones se realizan con la variable localArchives , en el siguiente bloque del mismo tipo, con localFiles . Y si estudia concienzudamente este código y no lo revisa rápidamente, como suele ser el caso con la revisión del código, observe el lugar donde olvidó reemplazar la variable localArchives .

Tal descuido podría conducir al siguiente escenario:

  • Supongamos que tenemos localArchives (size = 4) y localFiles (size = 2);
  • Al crear la matriz localFiles.toArray (new String [localArchives.size ()]) , obtenemos los últimos 2 elementos como nulos (["pathToFile1", "pathToFile2", null, null]);
  • Después de eso, org.apache.hadoop.util.StringUtils.arrayToString devolverá una representación de cadena de nuestra matriz en la que los últimos nombres de archivo se representarán como "nulo" ("pathToFile1, pathToFile2, null, null" ) ;
  • Todo esto se transmitirá, y quién sabe qué 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 comprobación del número de elementos en 0 se realiza por separado, la comprobación posterior de children.size ()> 0 siempre dará verdadero.

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

Este defecto radica en el hecho de que la variable se divide en sí misma. Como resultado, la verificación de multiplicidad siempre pasará y, en caso de datos de entrada incorrectos ( windowLenMs , numBuckets ), no se lanzará la excepción.

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

En dos ramas de casos , el mismo código fragmenta. ¡Esto sucede todo el tiempo! En el número predominante de casos, esto no es un error real, sino solo una ocasión para pensar en la refactorización del interruptor . Pero no para el caso en cuestión. Al repetir fragmentos de código, se establece el valor de la variable preemptedVcoreSeconds . Si presta atención a los nombres de todas las variables y constantes, puede llegar a la conclusión de que en el caso de metric.getId () == APP_MEM_PREEMPT_METRICS , el valor de la variable preemptedMemorySeconds debe establecerse, no adelantarse a VcoreSeconds . En este sentido, preemptedMemorySeconds siempre permanecerá 0 después de ejecutar la instrucción 'switch', y el valor de preemptedVcoreSeconds puede 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); } .... }

Variable planQueueName no utilizada al iniciar sesión. Aquí, copiaron demasiado o no modificaron la cadena de formato. Pero, sin embargo, me inclino por lo bueno y, a veces, perjudicar, copiar y pegar.

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 V6072. Tenga cuidado con 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? =). El cuerpo del bucle, que es parte de la prueba, nunca se ejecuta. Esto se debe al hecho de que los valores inicial y final del contador coinciden en la instrucción for . Como resultado, la condición i <start inmediatamente nos dará falso, lo que conducirá a este comportamiento. Revisé el archivo con las pruebas y llegué a la conclusión de que era necesario escribir en la condición del bucle i <(inicio + n) .

Mapreduce


a href = " www.viva64.com/en/w/v6007 "> 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 entender, vamos al código de arriba. Si la ejecución de la prueba alcanza la operación byteAm - = headerLen , esto significa que habrá byteAm> = headerLen . Desde aquí, después de realizar la resta, el valor de byteAm nunca será negativo. Lo cual se requería para probar.

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 " + ....); }

No lo creas, y de nuevo V6072! 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 highestPriorityLowRedundancyECBlocksStr se inicializan con los mismos valores. A menudo debería ser, pero no en esta situación. Los nombres de las variables aquí son largos y similares entre sí, por lo que no me sorprende que no haya modificaciones correspondientes con copiar y pegar. Para remediar la situación, al inicializar la variable más altaPriorityLowRedundancyECBlocksStr, debe usar el parámetro de entrada más altoPriorityLowRedundancyECBlocks . Además de esto, lo más probable es que aún necesite corregir la cadena 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 jura que cambiar el contador i ++ en el bucle es inalcanzable. Esto significa que en un bucle for (int i = 0; i <slowwriters.length; i ++) {....} no se realizará más de una iteración. Veamos por qué. Por lo tanto, en la primera iteración, asociamos la secuencia con el archivo correspondiente a escritores lentos [0] para leer más. A través del bucle for (int j = 0, x ;; j ++), leemos el contenido del archivo por byte, donde:

  • si leemos algo adecuado, entonces a través de afirmar Equivalentes comparamos el byte de lectura con el valor actual del contador j (en caso de verificación fallida, salimos de la prueba con error);
  • Si el archivo pasó la prueba y llegamos al final del archivo (leer -1), salimos del método.

Por lo tanto, no importa lo que suceda cuando revise escritores lentos [0] , no se procederá a la verificación de los siguientes elementos. Lo más probable es que se use 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, stacktrace nunca se imprimirá si ocurre una excepción, porque el método Assert.fail interrumpirá la prueba. Si hay suficiente mensaje de que se detecta la excepción, entonces para no confundirse, se debe eliminar la impresión de tracetrace'a. Si es necesario imprimir, solo necesita intercambiarlos.

Hay muchos lugares de este tipo:
  • 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, nuevamente V6072 =). Variables para familiarizarse con el fragmento sospechoso: usercache y publicCache .

Conclusión


Durante el desarrollo, se escriben cientos de miles de líneas de código. Si el código de producción intenta mantenerse limpio de errores, defectos y defectos (el desarrollador prueba su propio código, realiza una revisión del código y mucho más), entonces las pruebas son claramente inferiores a esto. Los defectos en las pruebas pueden ocultarse silenciosamente detrás de una "marca verde". Y como comprenderá por el análisis de advertencias de hoy, una prueba aprobada con éxito está lejos de ser siempre una prueba garantizada.

Al verificar la base de código de Apache Hadoop, el análisis estático demostró su necesidad no solo del código que entra en producción, sino también de las pruebas que también juegan un papel importante en el desarrollo.

Entonces, si le importa la calidad de su código y la base de prueba, le recomiendo que mire hacia el análisis estático. Y al primer solicitante de la prueba, le propongo probar PVS-Studio .



Si desea compartir este artículo con una audiencia de habla inglesa, utilice el enlace a la traducción: Maxim Stefanov. Apache Hadoop Code Quality: Production VS Test .

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

More articles:


All Articles