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 Testabdeckung bereitzustellen. Zweifellos müssen der Hauptprojektcode und die Tests in einem vollkommen zusammenhängenden Tandem zusammenarbeiten, um hohe Ergebnisse zu erzielen. Daher müssen Sie den Tests genau so viel Aufmerksamkeit schenken wie dem Hauptcode. Das Schreiben eines guten Tests ist der Schlüssel, um eine Regression in der Produktion zu erkennen. Um zu zeigen, wie wichtig es ist, dass Fehler in Tests nicht schlechter sind als in der Produktion, werden wir die nächste Analyse der Warnungen des statischen Analysators PVS-Studio in Betracht ziehen. Ziel: Apache Hadoop.

Über das Projekt


Diejenigen, die sich einst für Big Data interessierten, haben wahrscheinlich schon von einem Projekt wie Apache Hadoop gehört oder mit ihm gearbeitet. Kurz gesagt, Hadoop ist ein Framework, das als Grundlage für das Erstellen und Arbeiten 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 (verteiltes Hadoop-Dateisystem)
  • Garn

Es gibt jedoch viele Materialien, mit denen Sie sich im Internet vertraut machen können.

Über die Überprüfung


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

  • mit Maven Plugin;
  • Gradle Plugin verwenden;
  • Verwenden von IntellJ IDEA
  • Verwenden Sie den Analysator direkt.

Hadoop wurde auf der Basis des Maven-Build-Systems erstellt, so dass bei der Überprüfung keine Schwierigkeiten auftraten.

Nachdem das Skript aus der Dokumentation integriert und eines von pom.xml leicht angepasst wurde (es gab Module in den Abhängigkeiten, die nicht vorhanden waren), ging die Analyse!

Nach der Analyse, bei der Auswahl der interessantesten Warnungen, stellte ich fest, dass sowohl im Produktionscode als auch in den Tests die gleiche Anzahl von Warnungen vorhanden war. Normalerweise denke ich nicht über das Auslösen des Analysators nach, das auf Tests fällt. Aber als ich sie aufteilte, konnte ich die Warnungen aus der Kategorie "Tests", die hinter meiner Aufmerksamkeit lagen, nicht verfehlen. „Warum nicht?“, Dachte ich, denn Fehler in Tests haben auch Konsequenzen. Sie können zu fehlerhaften oder teilweisen Tests oder sogar zu Unsinn führen (nur zur Schau, damit sie immer grün sind).

Nachdem Sie die interessantesten Warnungen gesammelt und nach Code (Produktion, Test) und den vier Hauptmodulen von Hadoop unterteilt haben, möchte ich Sie auf eine Analyse der Funktionsweise des Analysegeräts aufmerksam machen.

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

Ein zweimaliger Mehrwert für ein HashSet ist ein häufiger Fehler beim Überprüfen von Projekten. Tatsächlich wird der zweite Zusatz ignoriert. Nun, wenn diese Vervielfältigung ein absurder Zufall ist. Was aber, wenn es wirklich bedeutete, einen weiteren Wert hinzuzufügen?

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

Diagnose V6072 macht manchmal sehr interessante Ergebnisse. Bei der Diagnose geht es im Wesentlichen darum, nach denselben Codefragmenttypen zu suchen, die durch Kopieren und Ersetzen von ein oder zwei Variablen erhalten wurden. Gleichzeitig wurden einige der Variablen jedoch „unterschätzt“.

Der obige Code demonstriert dies. Im ersten Block werden Aktionen mit der Variablen localArchives ausgeführt , im nächsten Block desselben Typs mit localFiles . Wenn Sie diesen Code gewissenhaft studieren und ihn nicht schnell durchgehen, wie dies bei der Codeüberprüfung häufig der Fall ist, notieren Sie sich die Stelle, an der Sie vergessen haben, die Variable localArchives zu ersetzen.

Ein solches Versehen könnte 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 ()]) werden die letzten beiden Elemente als null (["pathToFile1", "pathToFile2", null, null]);
  • Danach gibt org.apache.hadoop.util.StringUtils.arrayToString eine Zeichenfolgendarstellung unseres Arrays zurück, in der die letzten Dateinamen als "null" dargestellt werden ("pathToFile1, pathToFile2, null, null" ) ;
  • All dies wird weitergegeben, und wer weiß, welche Prüfungen 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) { .... } .... }

Aufgrund der Tatsache, dass die Überprüfung der Anzahl der Elemente bei 0 separat erfolgt, ergibt eine weitere Überprüfung von children.size ()> 0 immer true.

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

Dieser Mangel liegt darin, dass die Variable in sich selbst unterteilt ist. Infolgedessen wird die Prüfung auf Multiplizität immer bestanden, und im Falle falscher Eingabedaten ( windowLenMs , numBuckets ) wird die Ausnahme nicht 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; } } .... } .... }

In zwei Fällen werden dieselben Codefragmente verzweigt. Das passiert die ganze Zeit! In der überwiegenden Anzahl von Fällen handelt es sich nicht um einen echten Fehler, sondern nur um eine Gelegenheit, über das Umgestalten von Switches nachzudenken. Aber nicht für den fraglichen Fall. Wiederholende Codefragmente legen den Wert der Variablen preemptedVcoreSeconds fest . Wenn Sie auf die Namen aller Variablen und Konstanten achten, können Sie zu dem Schluss kommen, dass im Fall von metric.getId () == APP_MEM_PREEMPT_METRICS der Wert der Variablen preemptedMemorySeconds festgelegt werden sollte und nicht preemptedVcoreSeconds . In diesem Zusammenhang bleibt preemptedMemorySeconds nach Ausführung der Anweisung 'switch' immer auf 0, und der Wert von preemptedVcoreSeconds ist möglicherweise falsch.

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

Nicht verwendete Variable planQueueName bei der Protokollierung. Hier haben sie entweder zu viel kopiert oder die Formatzeichenfolge nicht geändert. Trotzdem neige ich zum guten alten und manchmal schlechten Kopieren und Einfügen.

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 V6072. Achten Sie auf die Variablen allSecretsA und allSecretsB .

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? =). Der Body der Schleife, der Teil des Tests ist, wird niemals ausgeführt. Dies liegt daran, dass die Start- und Endwerte des Zählers in der for- Anweisung übereinstimmen. Infolgedessen gibt die Bedingung i <start sofort false aus, was zu diesem Verhalten führt. Ich habe die Datei mit den Tests durchgesehen und bin zu dem Schluss gekommen, dass im Zustand der Schleife i <(start + n) geschrieben werden muss .

Mapreduce


a href = " www.viva64.com/de/w/v6007 "> 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 das zu verstehen, gehen wir den obigen Code durch. Wenn die Testausführung die Operation byteAm - = headerLen erreicht , bedeutet dies, dass es byteAm> = headerLen gibt . Ab hier ist der Wert von byteAm nach der Subtraktion niemals negativ. Welches war erforderlich, um zu 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 " + ....); }

Glauben Sie es nicht, und 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 sein, aber nicht in dieser Situation. Die Variablennamen hier sind lang und ähnlich, daher wundert es mich nicht, dass beim Kopieren und Einfügen keine entsprechenden Änderungen vorgenommen wurden. Um die Situation zu korrigieren, müssen Sie beim Initialisieren der Variablen highestPriorityLowRedundancyECBlocksStr den Eingabeparameter highestPriorityLowRedundancyECBlocks verwenden . Darüber hinaus müssen Sie höchstwahrscheinlich die Formatzeichenfolge 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 schwört, dass das Ändern des i ++ - Zählers in der Schleife nicht möglich ist. Dies bedeutet, dass in einer for- Schleife (int i = 0; i <slowwriters.length; i ++) {....} nicht mehr als eine Iteration ausgeführt wird. Finden wir heraus warum. In der ersten Iteration verknüpfen wir den Stream zum weiteren Lesen mit der Datei, die den Slowwritern [0] entspricht. Durch die for- Schleife (int j = 0, x ;; j ++) lesen wir den Inhalt der Datei byteweise, wobei:

  • Wenn wir etwas passendes lesen, vergleichen wir das gelesene Byte über assertEquals mit dem aktuellen Wert des Zählers j (wenn die Überprüfung nicht erfolgreich ist, verlassen wir den Test mit fail).
  • Wenn die Datei den Test bestanden hat und wir das Ende der Datei erreicht haben (read -1), verlassen wir die Methode.

Unabhängig davon , was beim Überprüfen von Slowwritern [0] geschieht, kommt es daher nicht zur Überprüfung der folgenden Elemente. Höchstwahrscheinlich sollte 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 wird die Stapelverfolgung niemals gedruckt, wenn eine Ausnahme auftritt, da die Assert.fail- Methode den Test unterbricht. Wenn genügend Meldungen vorliegen, dass die Ausnahme abgefangen wurde, muss der Stacktrace'a-Druck entfernt werden, um Verwirrung zu vermeiden. Wenn Drucken erforderlich ist, müssen Sie sie nur austauschen.

Es gibt viele solcher Orte:
  • 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, um sich mit einem verdächtigen Fragment vertraut zu machen: usercache und publiccache .

Fazit


Während der Entwicklung werden Hunderttausende von Codezeilen geschrieben. Wenn der Produktionscode versucht, Fehler, Mängel und Mängel zu beseitigen (der Entwickler testet seinen eigenen Code, führt eine Codeüberprüfung durch und vieles mehr), sind die Tests dem eindeutig unterlegen. Fehler in Tests können sich leise hinter einem "grünen Häkchen" verstecken. Und wie Sie aus der heutigen Analyse von Warnungen verstehen, ist ein erfolgreich bestandener Test keineswegs immer ein garantierter Test.

Bei der Überprüfung der Apache Hadoop-Codebasis hat die statische Analyse gezeigt, dass nicht nur der Code in der Produktion benötigt wird, sondern auch Tests, die auch bei der Entwicklung eine wichtige Rolle spielen.

Wenn Sie also Wert auf die Qualität Ihres Codes und Ihrer Testbasis legen, empfehle ich Ihnen, sich mit statischen Analysen zu befassen. Und der erste Bewerber für den Test, den ich vorschlage, PVS-Studio auszuprobieren.



Wenn Sie diesen Artikel mit einem englischsprachigen Publikum teilen möchten, verwenden Sie bitte den Link zur Übersetzung: Maxim Stefanov. Apache Hadoop Code Qualität: Produktion VS Test .

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

More articles:


All Articles