Geekly articles weekly

Apache Hadoop Code Quality: production VS test

Figure 1

Pour obtenir un code de production de haute qualité, il ne suffit pas de fournir une couverture de test maximale. Sans aucun doute, pour obtenir des résultats élevés, le code principal du projet et les tests doivent fonctionner en tandem parfaitement cohérent. Par conséquent, vous devez faire attention aux tests autant qu'au code principal. La rédaction d'un bon test est la clé pour rattraper une régression de la production. Pour montrer l'importance du fait que les bogues dans les tests ne sont pas pires qu'en production, nous considérerons la prochaine analyse des avertissements de l'analyseur statique PVS-Studio. Cible: Apache Hadoop.

À propos du projet


Ceux qui étaient autrefois intéressés par le Big Data ont probablement entendu parler ou travaillé avec un projet comme Apache Hadoop . En bref, Hadoop est un framework qui peut être utilisé comme base pour construire et travailler avec des systèmes Big Data.

Hadoop se compose de quatre modules principaux, chacun effectuant une tâche spécifique nécessaire à un système d'analyse de Big Data:

  • Hadoop commun
  • Mapreduce
  • Système de fichiers distribué Hadoop (Système de fichiers distribué Hadoop)
  • Fil

Cependant, il existe de nombreux documents pour vous familiariser avec Internet.

À propos de la vérification


Comme indiqué dans la documentation , PVS-Studio peut être intégré au projet de différentes manières:

  • en utilisant le plugin maven;
  • en utilisant le plugin gradle;
  • Utilisation d'IntellJ IDEA
  • en utilisant l'analyseur directement.

Hadoop a été construit sur la base du système de construction maven, il n'y a donc eu aucune difficulté avec la vérification.

Après avoir intégré le script de la documentation et légèrement ajusté un de pom.xml (il y avait des modules dans les dépendances qui n'étaient pas là), l'analyse est allée!

Après l'analyse, en choisissant les avertissements les plus intéressants, j'ai remarqué que j'avais le même nombre d'avertissements dans le code de production et les tests. Habituellement, je ne considère pas le déclenchement d'un analyseur qui tombe sur des tests. Mais, en les divisant, je ne pouvais pas manquer les avertissements de la catégorie «tests» devant mon attention. «Pourquoi pas?», Ai-je pensé, car les bogues dans les tests ont également des conséquences. Ils peuvent conduire à des tests incorrects ou partiels, voire à des bêtises (juste pour le spectacle, afin qu'ils soient toujours verts).

Ainsi, en rassemblant les avertissements les plus intéressants, en les divisant par code (production, test) et les quatre principaux modules Hadoop, je porte à votre attention une analyse du fonctionnement de l'analyseur.

Code de production


Hadoop commun


V6033 Un élément avec la même clé 'KDC_BIND_ADDRESS' a déjà été ajouté. 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); .... } .... }

Une valeur ajoutée deux fois à un HashSet est un défaut courant lors de la vérification de projets. En fait, le deuxième ajout sera ignoré. Eh bien, si cette duplication est un accident absurde. Et si cela signifiait vraiment ajouter une autre valeur?

Mapreduce


V6072 Deux fragments de code similaires ont été trouvés. Il s'agit peut-être d'une faute de frappe et la variable «localFiles» devrait être utilisée à la place 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 fait des résultats parfois très intéressants. L'essence du diagnostic est de rechercher le même type de fragments de code qui ont été obtenus par copier-coller et en remplaçant une ou deux variables, mais en même temps, certaines variables étaient «sous-estimées».

Le code ci-dessus le démontre. Dans le premier bloc, les actions sont effectuées avec la variable localArchives , dans le bloc suivant du même type, avec localFiles . Et si vous étudiez consciencieusement ce code et ne le passez pas en revue rapidement, comme c'est souvent le cas avec la révision de code, notez l'endroit où vous avez oublié de remplacer la variable localArchives .

Une telle omission pourrait conduire au scénario suivant:

  • Supposons que nous ayons localArchives (taille = 4) et localFiles (taille = 2);
  • Lors de la création du tableau localFiles.toArray (nouveau String [localArchives.size ()]) , nous obtenons que les 2 derniers éléments soient nuls (["pathToFile1", "pathToFile2", null, null]);
  • Après cela, org.apache.hadoop.util.StringUtils.arrayToString renverra une représentation sous forme de chaîne de notre tableau dans laquelle les derniers noms de fichiers seront représentés comme "null" ("pathToFile1, pathToFile2, null, null" ) ;
  • Tout cela sera transmis, et qui sait quels contrôles il y a pour de tels cas =).

V6007 L' expression 'children.size ()> 0' est toujours vraie. Queue.java (347)

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

Étant donné que la vérification du nombre d'éléments à 0 est effectuée séparément, une vérification supplémentaire de children.size ()> 0 donnera toujours la valeur true.

HDFS


V6001 Il existe des sous-expressions identiques 'this.bucketSize' à gauche et à droite de l'opérateur '%'. 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); } }

Ce défaut réside dans le fait que la variable est divisée en elle-même. Par conséquent, la vérification de la multiplicité passera toujours et, en cas de données d'entrée incorrectes ( windowLenMs , numBuckets ), l'exception ne sera pas levée.

Fil


V6067 Deux ou plusieurs branches de cas effectuent les mêmes actions. 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; } } .... } .... }

Dans deux branches de cas , les mêmes fragments de code. Cela arrive tout le temps! Dans le nombre prédominant de cas, ce n'est pas une vraie erreur, mais seulement une occasion de penser à une refactorisation des commutateurs . Mais pas pour le cas en question. Dans les extraits de code répétitifs, la valeur de la variable preemptedVcoreSeconds est définie . Si vous faites attention aux noms de toutes les variables et constantes, vous pouvez conclure que dans le cas de metric.getId () == APP_MEM_PREEMPT_METRICS , la valeur de la variable preemptedMemorySeconds doit être définie, et non preemptedVcoreSeconds . À cet égard, preemptedMemorySeconds restera toujours 0 après l'exécution de l'instruction 'switch', et la valeur de preemptedVcoreSeconds peut être incorrecte.

V6046 Format incorrect. Un nombre différent d'éléments de format est attendu. Arguments non utilisés: 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 inutilisée planQueueName lors de la connexion. Ici, ils ont trop copié ou n'ont pas modifié la chaîne de format. Mais néanmoins, je suis enclin au bon vieux et, en apportant parfois un mauvais service, au copier-coller.

Code de test


Hadoop commun


V6072 Deux fragments de code similaires ont été trouvés. Il s'agit peut-être d'une faute de frappe et la variable «allSecretsB» devrait être utilisée à la place 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]); .... }

Et encore V6072. Attention aux variables allSecretsA et allSecretsB .

V6043 Envisagez d'inspecter l'opérateur «pour». Les valeurs initiales et finales de l'itérateur sont identiques. 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); }

Un test toujours vert? =). Le corps de la boucle, qui fait partie du test, n'est jamais exécuté. Cela est dû au fait que les valeurs de début et de fin du compteur correspondent dans l'instruction for . Par conséquent, la condition i <start nous donnera immédiatement faux, ce qui conduira à ce comportement. J'ai parcouru le fichier avec les tests et suis arrivé à la conclusion qu'il était nécessaire d'écrire dans l'état de la boucle i <(start + n) .

Mapreduce


a href = " www.viva64.com/en/w/v6007 "> V6007 L'expression 'byteAm <0' est toujours fausse. 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 condition byteAm <0 est toujours fausse. Pour comprendre, remontons le code ci-dessus. Si l'exécution du test atteint l'opération byteAm - = headerLen , cela signifie qu'il y aura byteAm> = headerLen . À partir de là, après avoir effectué la soustraction, la valeur de byteAm ne sera jamais négative. Ce qui devait prouver.

HDFS


V6072 Deux fragments de code similaires ont été trouvés. Il s'agit peut-être d'une faute de frappe et la variable «normalFile» devrait être utilisée à la place 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 " + ....); }

Ne le croyez pas, et encore V6072! Suivez simplement les variables normalDir et normalFile

V6027 Les variables sont initialisées via l'appel à la même fonction. Il s'agit probablement d'une erreur ou d'un code non optimisé. 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); .... }

Dans ce fragment, les variables le plus élevéPriorityLowRedundancyReplicatedBlocksStr et le plus élevéPriorityLowRedundancyECBlocksStr sont initialisés avec les mêmes valeurs. C'est souvent le cas, mais pas dans cette situation. Les noms de variables ici sont longs et similaires les uns aux autres, donc je ne suis pas surpris qu'il n'y ait pas eu de modifications correspondantes avec le copier-coller. Pour remédier à la situation, lors de l'initialisation de la variable LowestPriorityLowRedundancyECBlocksStr, vous devez utiliser le paramètre d'entrée LowestPriorityLowRedundancyECBlocks . En plus de cela, très probablement, vous devez toujours corriger la chaîne de format.

V6019 Code inaccessible détecté. Il est possible qu'une erreur soit présente. 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); } } }

L'analyseur jure que la modification du compteur i ++ dans la boucle est impossible. Cela signifie que dans une boucle for (int i = 0; i <slowwriters.length; i ++) {....} pas plus d'une itération ne sera effectuée. Voyons pourquoi. Ainsi, dans la première itération, nous associons le flux au fichier correspondant aux ralentisseurs [0] pour une lecture plus approfondie. À travers la boucle for (int j = 0, x ;; j ++), nous lisons le contenu du fichier par octet, où:

  • si nous lisons quelque chose d'adéquat, alors via assertEquals nous comparons l'octet de lecture avec la valeur actuelle du compteur j (en cas de vérification infructueuse, nous quittons le test avec échec);
  • si le fichier a réussi le test et que nous avons atteint la fin du fichier (lire -1), nous quittons la méthode.

Par conséquent, peu importe ce qui se passe lors de la vérification des ralentisseurs [0] , il ne s'agit pas de vérifier les éléments suivants. Très probablement, la pause devrait être utilisée au lieu du retour .

Fil


V6019 Code inaccessible détecté. Il est possible qu'une erreur soit présente. TestNodeManager.java (176)

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

Dans cette situation, stacktrace ne sera jamais imprimé si une exception se produit, car la méthode Assert.fail interrompra le test. S'il y a suffisamment de message que l'exception est interceptée, alors pour ne pas être confus, l'impression stacktrace'a doit être supprimée. Si l'impression est nécessaire, il vous suffit de les échanger.

Il existe de nombreux endroits de ce type:
  • V6019 Code inaccessible détecté. Il est possible qu'une erreur soit présente. TestResourceTrackerService.java (928)
  • V6019 Code inaccessible détecté. Il est possible qu'une erreur soit présente. TestResourceTrackerService.java (737)
  • V6019 Code inaccessible détecté. Il est possible qu'une erreur soit présente. TestResourceTrackerService.java (685)
  • ....

V6072 Deux fragments de code similaires ont été trouvés. Il s'agit peut-être d'une faute de frappe et la variable «publicCache» devrait être utilisée à la place 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), ....); .... } .... }

Et enfin, encore V6072 =). Variables pour vous familiariser avec un fragment suspect: usercache et publicCache .

Conclusion


Pendant le développement, des centaines de milliers de lignes de code sont écrites. Si le code de production essaie de se maintenir à l'abri des bogues, des défauts et des lacunes (le développeur teste son propre code, procède à une révision du code, et bien plus encore), alors les tests sont clairement inférieurs à cela. Les défauts dans les tests peuvent se cacher discrètement derrière une "coche verte". Et comme vous le comprenez d'après l'analyse des avertissements d'aujourd'hui, un test réussi est loin d'être toujours un test garanti.

Lors de la vérification de la base de code Apache Hadoop, l'analyse statique a démontré son besoin non seulement pour le code qui entre en production, mais aussi pour les tests qui jouent également un rôle important dans le développement.

Donc, si vous vous souciez de la qualité de votre code et de votre base de test, je vous recommande de vous tourner vers l'analyse statique. Et le premier candidat au test, je propose d'essayer PVS-Studio .



Si vous souhaitez partager cet article avec un public anglophone, veuillez utiliser le lien vers la traduction: Maxim Stefanov. Apache Hadoop Code Quality: Production VS Test .

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

More articles:


All Articles