Geekly articles weekly

Apache Hadoop代码质量：生产VS测试

图1

为了获得高质量的生产代码，仅确保最大的测试覆盖范围还不够。 毫无疑问，出色的结果需要主要的项目代码和测试有效地协同工作。 因此，测试必须与主要代码一样受到重视。 体面的测试是成功的关键因素，因为它将赶上生产的衰退。 让我们看一下PVS-Studio静态分析器警告，以查看测试错误不会比生产错误严重这一事实的重要性。 当今的焦点：Apache Hadoop。

关于项目


那些以前对大数据感兴趣的人可能已经听说过甚至与Apache Hadoop项目合作。 简而言之，Hadoop是一个框架，可以用作构建大数据系统和使用它们的基础。

Hadoop由四个主要模块组成，每个模块执行大数据分析系统所需的特定任务：

  • Hadoop常见
  • Mapreduce
  • Hadoop分布式文件系统
  • 纱线

无论如何，Internet上有很多有关它的资料。

关于支票


如文档中所示，PVS-Studio可以通过多种方式集成到项目中：

  • 使用maven插件；
  • 使用gradle插件；
  • 使用gratell IntellJ IDEA；
  • 直接使用分析仪。

Hadoop基于maven构建系统，因此，检查没有障碍。

在集成了文档中的脚本并编辑了一个pom.xml文件（依赖项中有一些模块不可用）之后，分析开始了！

分析完成后，我选择了最有趣的警告，并注意到在生产代码和测试中，我具有相同数量的警告。 通常，我不考虑针对测试提供的分析器警告。 但是，当我将它们分开时，我无法不理会“测试”警告。 “为什么不看一下它们？”-我想，因为测试中的错误可能还会带来不利的后果。 它们可能导致错误或部分测试，甚至混搭（它们存在的目的只是为了选中复选框，它们始终是绿色的）。

选择最有趣的警告后，我将其分为以下几类：生产，测试和四个主要Hadoop模块。 现在，我很高兴为您提供对分析仪警告的复审。

生产代码


Hadoop常见


V6033已经添加了具有相同键“ 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); .... } .... }

检查项目时，在HashSet中两次添加的值是一个非常常见的缺陷。 实际上，第二个添加项将被忽略。 我希望这种重复只是一场不必要的悲剧。 如果要添加另一个值怎么办？

Mapreduce


V6072找到两个相似的代码片段。 也许这是一个错字，应该使用“ localFiles”变量而不是“ 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()]))); } .... }

V6072诊断程序有时会产生一些有趣的发现。 此诊断的目的是检测由于复制粘贴和替换两个变量而导致的相同类型的代码片段。 在这种情况下，某些变量甚至保持“不变”。

上面的代码演示了这一点。 在第一个块中，使用localArchives变量，在第二个类似的片段中，使用localFiles 。 如果您认真研究此代码，而不是很快地遍历它，因为它经常在代码审阅时发生，您会注意到该片段，作者忘记了替换localArchives变量。

这种失态可能导致以下情况：

  • 假设我们有localArchives （大小= 4）和localFiles （大小= 2）；
  • 创建数组localFiles.toArray（新字符串[localArchives.size（）]）时 ，最后2个元素将为null （[[“ pathToFile1”，“ pathToFile2”，null，null]）；
  • 然后org.apache.hadoop.util.StringUtils.arrayToString将返回数组的字符串表示形式，其中最后一个文件名将显示为“ null”（“ pathToFile1，pathToFile2，null，null” ）） ；
  • 所有这一切将进一步传递，上帝只知道这种情况下会有什么样的检查。

V6007表达式'children.size（）> 0'始终为true。 Queue.java（347）

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

由于要单独检查元素数是否为0，因此进一步检查children.size（）> 0始终为true。

HDFS


V6001在'％'运算符的左侧和右侧有相同的子表达式'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); } }

此处的缺陷是变量被自身除。 结果，对多重性的检查将始终成功，并且在获得不正确的输入（ windowLenMs ， numBuckets ）的情况下，永远不会引发异常。

纱线


V6067两个或更多案件分支执行相同的操作。 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; } } .... } .... }

两个案例分支中的相同代码片段。 到处都是！ 在大多数情况下，这不是真正的错误，而只是考虑对switch进行重构的一个原因。 但目前情况并非如此。 重复的代码片段设置变量preemptedVcoreSeconds的值。 如果仔细查看所有变量和常量的名称，您可能会得出结论，如果metric.getId（）== APP_MEM_PREEMPT_METRICS，则必须为preemptedMemorySeconds变量而不是preemptedVcoreSeconds设置值。 在这方面，在“ switch”运算符之后， preemptedMemorySeconds将始终保持为0，而preemptedVcoreSeconds的值可能不正确。

V6046格式错误。 预计会有不同数量的格式项。 不使用的参数：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); } .... }

记录时不使用planQueueName变量。 在这种情况下，要么复制太多，要么格式字符串未完成。 但是我仍然要责备旧的复制粘贴，在某些情况下，将其粘贴在脚上真是太好了。

测试码


Hadoop常见


V6072找到两个相似的代码片段。 也许这是一个错字，应该使用'allSecretsB'变量而不是'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]); .... }

再次是V6072。 仔细查看变量allSecretsA和allSecretsB 。

V6043考虑检查“ for”运算符。 迭代器的初始值和最终值相同。 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); }

总是绿色的测试？ =）。 循环的一部分，即测试本身的一部分，将永远不会执行。 这是由于以下事实： for语句中的初始计数器值和最终计数器值相等。 结果，条件i <开始将立即变为假，从而导致这种行为。 我浏览了测试文件，并得出结论，实际上i <（start + n）必须在循环条件下编写。

Mapreduce


V6007表达式'byteAm <0'始终为false。 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; } .... }

条件byteAm <0始终为false。 为了弄清楚，让我们再看一下代码。 如果测试执行到达操作byteAm-= headerLen ，则意味着byteAm> = headerLen。 从这里开始，减去后， byteAm值将永远不会为负。 那就是我们必须证明的。

HDFS


V6072找到两个相似的代码片段。 也许这是一个错字，应该使用'normalFile'变量而不是'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 " + ....); }

信不信由你，又是V6072！ 只需遵循变量normalDir和normalFile。

V6027通过调用同一函数来初始化变量。 可能是错误或未优化的代码。 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); .... }

在此片段中，变量highestPriorityLowRedundancyReplicatedBlocksStr和highestPriorityLowRedundancyECBlocksStr用相同的值初始化。 通常应该是这样，但事实并非如此。 变量的名称又长又相似，因此复制粘贴的片段没有进行任何更改也就不足为奇了。 要解决此问题，在初始化maximumPriorityLowRedundancyECBlocksStr变量时，作者必须使用输入参数maximumPriorityLowRedundancyECBlocks 。 此外，最有可能的是，他们仍然需要更正格式行。

V6019检测不到代码。 可能存在错误。 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); } } }

分析器抱怨无法更改循环中的i ++计数器。 这意味着在for（int i = 0; i <slowwriters.length; i ++）{....}循环中，最多将执行一次迭代。 让我们找出原因。 因此，在第一次迭代中，我们将线程与慢速写入器[0]对应的文件链接起来，以进行进一步的读取。 接下来，我们通过循环读取文件内容（int j = 0，x ;; j ++）：

  • 如果我们读取了一些相关的东西，我们将通过assertEquals将读取的字节与j计数器的当前值进行比较（如果检查不成功，则测试失败）；
  • 如果文件检查成功，并且到达文件末尾（读取为-1），则该方法退出。

因此，在检查lowwriters [0]期间发生的任何事情，都不会去检查后续元素。 最有可能的是，必须使用中断来代替返回。

纱线


V6019检测不到代码。 可能存在错误。 TestNodeManager.java（176）

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

在这种情况下， Assert.fail方法将中断测试，并且在出现异常的情况下不会打印stacktrace。 如果有关捕获到的异常的消息在这里足够多了，那么最好取消打印stacktrace以避免混淆。 如果需要打印，则只需交换它们。

已发现许多类似的片段：

  • V6019检测不到代码。 可能存在错误。 TestResourceTrackerService.java（928）
  • V6019检测不到代码。 可能存在错误。 TestResourceTrackerService.java（737）
  • V6019检测不到代码。 可能存在错误。 TestResourceTrackerService.java（685）
  • ....

V6072找到两个相似的代码片段。 也许这是一个错字，应该使用“ publicCache”变量而不是“ 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), ....); .... } .... }

最后，再次是V6072 =）。 用于查看可疑片段的变量： usercache和publicCache 。

结论


开发中编写了成千上万的代码行。 生产代码通常保持清洁，没有错误，缺陷和缺陷（开发人员测试他们的代码，检查代码等）。 在这方面，测试肯定不如。 测试中的缺陷很容易隐藏在“绿色刻度”后面。 正如您可能从今天的警告回顾中了解到的那样，绿色测试并不总是一项成功的检查。

这次，当检查Apache Hadoop代码库时，在生产代码和测试中都非常需要静态分析，而静态分析在开发中也起着重要作用。

因此，如果您关心代码并测试质量，建议您着眼于静态分析。 让PVS-Studio成为这项工作的第一竞争者。

Source: https://habr.com/ru/post/zh-CN480918/

More articles:


All Articles