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 d'assurer une couverture maximale avec les tests. Aucun doute, de bons résultats nécessitent que le code principal du projet et les tests fonctionnent efficacement ensemble. Par conséquent, les tests doivent recevoir autant d'attention que le code principal. Un test décent est un facteur clé de succès, car il entraînera une régression de la production. Jetons un coup d'œil aux avertissements de l'analyseur statique PVS-Studio pour voir l'importance du fait que les erreurs dans les tests ne sont pas pires que celles en production. Objectif d'aujourd'hui: Apache Hadoop.

À propos du projet


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

Hadoop se compose de quatre modules principaux, chacun d'eux effectue une tâche spécifique requise pour un système d'analyse de Big Data:

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

Quoi qu'il en soit, il existe de nombreux documents à ce sujet sur Internet.

À propos du chèque


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

  • Utilisation du plugin maven;
  • Utilisation du plugin gradle;
  • Utilisation du Gradle IntellJ IDEA;
  • Utilisation directe de l'analyseur.

Hadoop est basé sur le système de construction maven, par conséquent, il n'y avait aucun obstacle avec le chèque.

Après avoir intégré le script de la documentation et édité l'un des fichiers pom.xml (il y avait des modules dans des dépendances qui n'étaient pas disponibles), l'analyse a commencé!

Une fois l'analyse terminée, j'ai choisi les avertissements les plus intéressants et j'ai remarqué que j'avais le même nombre d'avertissements dans le code de production et dans les tests. Normalement, je ne considère pas les avertissements de l'analyseur, donnés pour les tests. Mais quand je les ai divisés, je ne pouvais pas laisser les avertissements «tests» sans surveillance. «Pourquoi ne pas y jeter un coup d'œil?» - pensais-je, car les bogues dans les tests pouvaient également avoir des conséquences néfastes. Ils peuvent conduire à des tests incorrects ou partiels, voire à des méli-mélo (ils existent juste pour cocher la case, ils sont toujours verts).

Après avoir sélectionné les avertissements les plus intrigants, je les ai divisés par les groupes suivants: production, test et les quatre principaux modules Hadoop. Et maintenant, je suis heureux d'offrir à votre attention l'examen des avertissements 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); .... } .... }

La valeur ajoutée deux fois dans HashSet est un défaut très courant lors de la vérification des projets. Le deuxième ajout sera en fait ignoré. J'espère que cette duplication n'est qu'une tragédie inutile. Et si une autre valeur devait être ajoutée?

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()]))); } .... }

Le diagnostic V6072 donne parfois des résultats intéressants. Le but de ce diagnostic est de détecter les fragments de code de même type résultant du copier-coller et du remplacement de une à deux variables. Dans ce cas, certaines variables sont même restées "sous-modifiées".

Le code ci-dessus le démontre. Dans le premier bloc, la variable localArchives est utilisée, dans le deuxième fragment similaire - localFiles . Si vous étudiez ce code avec une diligence raisonnable, plutôt que de le parcourir rapidement, comme cela arrive souvent lors de la révision du code, vous remarquerez le fragment, où l'auteur a oublié de remplacer la variable localArchives .

Cette gaffe peut 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 (new String [localArchives.size ()]) , les 2 derniers éléments seront nuls (["pathToFile1", "pathToFile2", null, null]);
  • Ensuite, org.apache.hadoop.util.StringUtils.arrayToString renverra la représentation sous forme de chaîne de notre tableau, dans laquelle les derniers noms de fichiers seront présentés comme "null" ("pathToFile1, pathToFile2, null, null" ) ;
  • Tout cela sera passé plus loin et Dieu sait seulement quel genre de 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 le nombre d'éléments est vérifié séparément pour 0, la vérification supplémentaire children.size ()> 0 sera toujours vraie.

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

Ici, le défaut est que la variable est divisée par elle-même. Par conséquent, la vérification de la multiplicité sera toujours réussie et en cas d'obtention d'entrées incorrectes ( windowLenMs , numBuckets ), l'exception ne sera jamais 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; } } .... } .... }

Mêmes fragments de code dans deux branches de cas . C'est juste partout! Dans le nombre actuel de cas, il ne s'agit pas d'une véritable erreur, mais simplement d'une raison de penser à une refactorisation du commutateur . Mais pas pour le cas qui nous occupe. Des fragments de code répétés définissent la valeur de la variable preemptedVcoreSeconds . Si vous examinez attentivement les noms de toutes les variables et constantes, vous conclurez probablement que si métric.getId () == APP_MEM_PREEMPT_METRICS, la valeur doit être définie pour la variable preemptedMemorySeconds , et non pour preemptedVcoreSeconds . À cet égard, après l'opérateur «switch», preemptedMemorySeconds restera toujours 0, tandis que 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); } .... }

La variable planQueueName n'est pas utilisée lors de la journalisation. Dans ce cas, trop est copié ou la chaîne de formatage n'est pas terminée. Mais je blâme toujours le bon vieux copier-coller, qui dans certains cas est tout simplement génial pour se tirer une balle dans le pied.

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 le V6072. Examinez attentivement les 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? =). Une partie de la boucle, qui fait partie du test lui-même, ne sera jamais exécutée. Cela est dû au fait que les valeurs de compteur initiales et finales sont égales dans l'instruction for . En conséquence, la condition i <start deviendra immédiatement fausse, conduisant à un tel comportement. J'ai parcouru le fichier de test et j'ai sauté à la conclusion qu'en réalité, i <(start + n) devait être écrit dans la condition de boucle.

Mapreduce


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 le comprendre, donnons le code au-dessus d'un autre coup d'œil. Si l'exécution du test atteint l'opération byteAm - = headerLen , cela signifie que byteAm> = headerLen. À partir de là, après soustraction, la valeur octetAm ne sera jamais négative. C'est ce que nous devions 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 " + ....); }

Croyez-le ou non, c'est encore le 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 par les mêmes valeurs. Il devrait souvent en être ainsi, mais ce n'est pas le cas. Les noms des variables sont longs et similaires, il n'est donc pas surprenant que le fragment copié-collé n'ait été modifié d'aucune façon. Pour y remédier, lors de l'initialisation de la variable le plus élevéPriorityLowRedundancyECBlocksStr , l'auteur doit utiliser le paramètre d'entrée le plus élevéPriorityLowRedundancyECBlocks . En outre, très probablement, ils doivent toujours corriger la ligne 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 se plaint que le compteur i ++ dans la boucle ne peut pas être modifié. Ce qui signifie que dans la boucle for (int i = 0; i <slowwriters.length; i ++) {....} pas plus d'une itération ne s'exécutera. Voyons pourquoi. Ainsi, dans la première itération, nous lions le thread avec le fichier qui correspond à slowwriters [0] pour une lecture plus approfondie. Ensuite, nous lisons le contenu du fichier via la boucle pour (int j = 0, x ;; j ++):

  • si nous lisons quelque chose de pertinent, nous comparons l'octet de lecture avec la valeur actuelle du compteur j via assertEquals (si la vérification échoue, nous échouons au test);
  • si le fichier est vérifié avec succès et que nous arrivons à la fin du fichier (nous lisons -1), la méthode se termine.

Par conséquent, quoi qu'il arrive lors de la vérification des ralentisseurs [0] , il ne sera pas possible de vérifier les éléments suivants. Très probablement, la pause doit ê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, la méthode Assert.fail interrompra le test et stacktrace ne sera pas imprimé en cas d'exception. Si le message concernant l'exception interceptée est suffisant ici, il est préférable de supprimer l'impression de stacktrace pour éviter toute confusion. Si l'impression est nécessaire, il vous suffit de les échanger.

De nombreux fragments similaires ont été trouvés:

  • 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, V6072 à nouveau =). Variables pour afficher le fragment suspect: usercache et publicCache .

Conclusion


Des centaines de milliers de lignes de code sont écrites en cours de développement. Le code de production est généralement conservé à l'abri des bogues, des défauts et des défauts (les développeurs testent leur code, le code est révisé, etc.). Les tests sont nettement inférieurs à cet égard. Les défauts dans les tests peuvent facilement se cacher derrière une "coche verte". Comme vous l'avez probablement compris dans l'examen des avertissements d'aujourd'hui, un test vert n'est pas toujours une vérification réussie.

Cette fois, lors de la vérification de la base de code Apache Hadoop, l'analyse statique s'est avérée très nécessaire à la fois dans le code de production et dans les tests qui jouent également un rôle important dans le développement.

Donc, si vous vous souciez de votre code et de la qualité des tests, je vous suggère de viser l'analyse statique. Que PVS-Studio soit le premier concurrent de cette entreprise.

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

More articles:


All Articles