Geekly articles weekly

Apache Hadoop Code Qualität: Produktion VS Test

Abbildung 1

Um qualitativ hochwertigen Produktionscode zu erhalten, reicht es nicht aus, nur eine maximale Abdeckung mit Tests zu gewährleisten. Keine Zweifel, gute Ergebnisse erfordern, dass der Hauptprojektcode und die Tests effizient zusammenarbeiten. Tests müssen daher genauso beachtet werden wie der Hauptcode. Ein anständiger Test ist ein wichtiger Erfolgsfaktor, da er die Regression in der Produktion auffängt. Werfen wir einen Blick auf die Warnungen des PVS-Studio-Statikanalysators, um festzustellen, wie wichtig es ist, dass Fehler in Tests nicht schlechter sind als in der Produktion. Der heutige Fokus: Apache Hadoop.

Über das Projekt


Diejenigen, die sich früher für Big Data interessierten, haben wahrscheinlich das Apache Hadoop- Projekt gehört oder sogar mit ihm zusammengearbeitet. Kurz gesagt, Hadoop ist ein Framework, das als Grundlage für den Aufbau und die Arbeit mit Big Data-Systemen verwendet werden kann.

Hadoop besteht aus vier Hauptmodulen, die jeweils eine bestimmte Aufgabe erfüllen, die für ein Big-Data-Analysesystem erforderlich ist:

  • Hadoop gemeinsam
  • Mapreduce
  • Hadoop Distributed File System
  • Garn

Jedenfalls gibt es im Internet jede Menge Materialien dazu.

Über den Scheck


Wie in der Dokumentation gezeigt, kann PVS-Studio auf verschiedene Arten in das Projekt integriert werden:

  • Verwenden des Maven-Plugins;
  • Verwenden des Gradle-Plugins;
  • Verwenden der gradle IntellJ IDEA;
  • Direkt mit dem Analysator.

Hadoop basiert auf dem Maven-Build-System, daher gab es keine Hindernisse bei der Überprüfung.

Nachdem ich das Skript aus der Dokumentation integriert und eine der pom.xml-Dateien bearbeitet hatte (es gab Module in Abhängigkeiten, die nicht verfügbar waren), wurde die Analyse gestartet!

Nachdem die Analyse abgeschlossen war, wählte ich die interessantesten Warnungen aus und stellte fest, dass ich im Produktionscode und in Tests die gleiche Anzahl von Warnungen hatte. Normalerweise berücksichtige ich keine Warnungen des Analysegeräts, die für Tests ausgegeben wurden. Aber als ich sie teilte, konnte ich die Warnungen vor "Tests" nicht unbeaufsichtigt lassen. „Warum sie sich nicht ansehen?“ - dachte ich, denn Fehler in Tests könnten auch nachteilige Folgen haben. Sie können zu inkorrekten oder teilweisen Tests oder sogar zu Mischmasch führen (sie existieren nur, um das Kontrollkästchen zu aktivieren, dass sie immer grün sind).

Nachdem ich die interessantesten Warnungen ausgewählt hatte, teilte ich sie in die folgenden Gruppen ein: Produktion, Test und die vier Hauptmodule von Hadoop. Und jetzt bin ich froh, Ihnen die Überprüfung der Warnungen des Analysegeräts anbieten zu können.

Produktionscode


Hadoop gemeinsam


V6033 Ein Artikel mit demselben Schlüssel 'KDC_BIND_ADDRESS' wurde bereits hinzugefügt. 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); .... } .... }

Der doppelte Mehrwert in HashSet ist ein sehr häufiger Fehler bei der Überprüfung von Projekten. Der zweite Zusatz wird ignoriert. Ich hoffe, diese Vervielfältigung ist nur eine unnötige Tragödie. Was ist, wenn ein anderer Wert hinzugefügt werden soll?

Mapreduce


V6072 Es wurden zwei ähnliche Codefragmente gefunden. Möglicherweise ist dies ein Tippfehler, und die Variable 'localFiles' sollte anstelle von 'localArchives' verwendet werden. 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()]))); } .... }

Die V6072-Diagnose liefert manchmal einige interessante Ergebnisse. Der Zweck dieser Diagnose besteht darin, identische Codefragmente zu erkennen, die beim Kopieren und Ersetzen von ein bis zwei Variablen entstanden sind. In diesem Fall wurden einige Variablen sogar "unterbewertet" gelassen.

Der obige Code demonstriert dies. Im ersten Block wird die Variable localArchives verwendet, im zweiten ähnlichen Fragment localFiles . Wenn Sie diesen Code mit der gebotenen Sorgfalt studieren und ihn dann schnell durcharbeiten , wie dies häufig bei der Codeüberprüfung der Fall ist, werden Sie das Fragment bemerken, bei dem der Autor vergessen hat, die Variable localArchives zu ersetzen.

Diese Gaffe kann zu folgendem Szenario führen:

  • Angenommen, wir haben localArchives (Größe = 4) und localFiles (Größe = 2);
  • Beim Erstellen des Arrays localFiles.toArray (neuer String [localArchives.size ()]) sind die letzten beiden Elemente null (["pathToFile1", "pathToFile2", null, null]);
  • Dann gibt org.apache.hadoop.util.StringUtils.arrayToString die Zeichenfolgendarstellung unseres Arrays zurück, in der die letzten Dateinamen als "null" ("pathToFile1, pathToFile2, null, null" ) dargestellt werden .
  • All dies wird weitergegeben und Gott weiß nur, welche Art von Kontrollen es für solche Fälle gibt =).

V6007 Ausdruck 'children.size ()> 0' ist immer wahr. Queue.java (347)

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

Da die Anzahl der Elemente separat auf 0 geprüft wird, ist die weitere Prüfung children.size ()> 0 immer wahr.

HDFS


V6001 Links und rechts vom Operator '%' gibt es identische Unterausdrücke 'this.bucketSize'. 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); } }

Hier besteht der Mangel darin, dass die Variable durch sich selbst geteilt wird. Infolgedessen ist die Prüfung auf Multiplizität immer erfolgreich, und im Falle falscher Eingaben ( windowLenMs , numBuckets ) wird die Ausnahme niemals ausgelöst.

Garn


V6067 Zwei oder mehr Fallzweige führen die gleichen Aktionen aus. 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; } } .... } .... }

Gleiche Codefragmente in zwei case Zweigen. Es ist einfach überall! In der überwiegenden Anzahl von Fällen handelt es sich nicht um einen echten Fehler, sondern nur um einen Grund, über eine Umgestaltung des Switch nachzudenken. Aber nicht für den vorliegenden Fall. Wiederholte Codefragmente setzen den Wert der Variablen preemptedVcoreSeconds . Wenn Sie sich die Namen aller Variablen und Konstanten genau ansehen , werden Sie wahrscheinlich den Schluss ziehen, dass im Fall von metric.getId () == APP_MEM_PREEMPT_METRICS der Wert für die Variable preemptedMemorySeconds festgelegt werden muss, nicht für preemptedVcoreSeconds . In dieser Hinsicht bleibt preemptedMemorySeconds nach dem Operator 'switch' immer auf 0, während der Wert von preemptedVcoreSeconds möglicherweise falsch ist.

V6046 Falsches Format. Eine andere Anzahl von Formatelementen wird erwartet. Nicht verwendete Argumente: 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); } .... }

Die Variable planQueueName wird bei der Protokollierung nicht verwendet. In diesem Fall wird entweder zu viel kopiert oder die Formatzeichenfolge ist nicht fertig. Aber ich beschuldige immer noch die gute alte Kopierpaste, die in manchen Fällen einfach großartig ist, um sich in den Fuß zu schießen.

Code testen


Hadoop gemeinsam


V6072 Es wurden zwei ähnliche Codefragmente gefunden. Möglicherweise ist dies ein Tippfehler, und die Variable 'allSecretsB' sollte anstelle von 'allSecretsA' verwendet werden. 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]); .... }

Und wieder das V6072. Schauen Sie sich die Variablen allSecretsA und allSecretsB genau an.

V6043 Betrachten Sie den Operator 'für'. Anfangs- und Endwert des Iterators sind gleich. 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); }

Ein Test, der immer grün ist? =). Ein Teil der Schleife, der Teil des Tests selbst ist, wird niemals ausgeführt. Dies liegt daran, dass der Anfangs- und der Endzählerwert in der for- Anweisung gleich sind. Infolgedessen wird die Bedingung i <start sofort falsch und führt zu einem solchen Verhalten. Ich habe die Testdatei durchgearbeitet und bin zu dem Schluss gekommen, dass i <(start + n) tatsächlich in der Schleifenbedingung geschrieben werden musste.

Mapreduce


V6007 Der Ausdruck 'byteAm <0' ist immer falsch. 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; } .... }

Die Bedingung byteAm <0 ist immer falsch. Um es herauszufinden, geben wir den Code über einen anderen Blick. Wenn die Testausführung die Operation byteAm - = headerLen erreicht , bedeutet dies, dass byteAm> = headerLen. Ab hier wird der ByteAm- Wert nach der Subtraktion niemals negativ sein. Das mussten wir beweisen.

HDFS


V6072 Es wurden zwei ähnliche Codefragmente gefunden. Möglicherweise ist dies ein Tippfehler, und die Variable 'normalFile' sollte anstelle von 'normalDir' verwendet werden. 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 " + ....); }

Ob Sie es glauben oder nicht, es ist wieder V6072! Folgen Sie einfach den Variablen normalDir und normalFile.

V6027 Variablen werden durch Aufruf derselben Funktion initialisiert. Es ist wahrscheinlich ein Fehler oder nicht optimierter Code. 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); .... }

In diesem Fragment werden die Variablen highestPriorityLowRedundancyReplicatedBlocksStr und highestPriorityLowRedundancyECBlocksStr mit denselben Werten initialisiert. Oft sollte es so sein, aber das ist nicht der Fall. Variablennamen sind lang und ähnlich, so dass es nicht verwunderlich ist, dass das kopierte Fragment in keiner Weise geändert wurde. Um dies zu beheben, muss der Autor beim Initialisieren der Variable highestPriorityLowRedundancyECBlocksStr den Eingabeparameter highestPriorityLowRedundancyECBlocks verwenden . Darüber hinaus müssen sie höchstwahrscheinlich die Formatzeile noch korrigieren.

V6019 Nicht erreichbarer Code erkannt. Möglicherweise liegt ein Fehler vor. 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); } } }

Der Analysator beschwert sich, dass der i ++ - Zähler in der Schleife nicht geändert werden kann. Dies bedeutet, dass in der for (int i = 0; i <slowwriters.length; i ++) {....} -Schleife nicht mehr als eine Iteration ausgeführt wird. Finden wir heraus warum. In der ersten Iteration verknüpfen wir den Thread zum weiteren Lesen mit der Datei, die Slowwriters [0] entspricht. Als nächstes lesen wir den Dateiinhalt über die Schleife nach (int j = 0, x ;; j ++):

  • Wenn wir etwas Relevantes lesen, vergleichen wir das gelesene Byte mit dem aktuellen Wert des Zählers j über assertEquals (wenn die Prüfung nicht erfolgreich ist, schlagen wir den Test fehl).
  • Wenn die Datei erfolgreich überprüft wurde und das Dateiende erreicht ist (wir lesen -1), wird die Methode beendet.

Unabhängig davon , was bei der Prüfung von Slowwritern [0] passiert, werden nachfolgende Elemente nicht geprüft. Höchstwahrscheinlich muss break anstelle von return verwendet werden.

Garn


V6019 Nicht erreichbarer Code erkannt. Möglicherweise liegt ein Fehler vor. TestNodeManager.java (176)

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

In diesem Fall unterbricht die Assert.fail- Methode den Test und die Stapelverfolgung wird im Falle einer Ausnahme nicht gedruckt. Wenn die Meldung über die gefangene Ausnahme hier ausreicht, sollten Sie das Drucken der Stapelverfolgung entfernen, um Verwirrung zu vermeiden. Wenn Sie drucken müssen, müssen Sie sie nur austauschen.

Viele ähnliche Fragmente wurden gefunden:

  • V6019 Nicht erreichbarer Code erkannt. Möglicherweise liegt ein Fehler vor. TestResourceTrackerService.java (928)
  • V6019 Nicht erreichbarer Code erkannt. Möglicherweise liegt ein Fehler vor. TestResourceTrackerService.java (737)
  • V6019 Nicht erreichbarer Code erkannt. Möglicherweise liegt ein Fehler vor. TestResourceTrackerService.java (685)
  • ....

V6072 Es wurden zwei ähnliche Codefragmente gefunden. Möglicherweise ist dies ein Tippfehler, und die Variable 'publicCache' sollte anstelle von 'usercache' verwendet werden. 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), ....); .... } .... }

Und zum Schluss nochmal V6072 =). Variablen zum Anzeigen des verdächtigen Fragments: usercache und publicCache .

Fazit


Hunderttausende von Codezeilen werden in der Entwicklung geschrieben. Produktionscode wird normalerweise von Fehlern, Defekten und Mängeln freigehalten (Entwickler testen ihren Code, der Code wird überprüft usw.). Tests sind in dieser Hinsicht definitiv unterlegen. Fehler in Tests können sich leicht hinter einem "grünen Häkchen" verstecken. Wie Sie wahrscheinlich aus der heutigen Überprüfung der Warnungen erfahren haben, ist ein grüner Test nicht immer eine erfolgreiche Prüfung.

Bei der Überprüfung der Apache Hadoop-Codebasis stellte sich diesmal heraus, dass statische Analysen sowohl im Produktionscode als auch in Tests, die auch bei der Entwicklung eine wichtige Rolle spielen, dringend erforderlich sind.

Wenn Sie sich also um Ihren Code kümmern und die Qualität testen, empfehle ich Ihnen, die statische Analyse im Auge zu behalten. Lassen Sie PVS-Studio der erste Anwärter in diesem Unternehmen sein.

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

More articles:


All Articles