# Качество кода Apache Hadoop: production VS test

Для того, чтобы получить качественный production код, недостаточно просто обеспечить максимальное покрытие тестами\. Несомненно, для того, чтобы добиться высоких результатов, основной код проекта и тесты обязаны работать в идеально сплоченном тандеме\. Поэтому уделять внимания тестам нужно столько же, сколько и основному коду\. Написание хорошего теста – залог того, что он отловит регрессию в production\. Чтобы показать важность того, что баги в тестах ничем не хуже, чем в production, рассмотрим очередной разбор предупреждений статического анализатора PVS\-Studio\. Цель: Apache Hadoop\.   

![0697_ApacheHadoop_ru/image1.png](https://import.viva64.com/docx/blog/0697_ApacheHadoop_ru/image1.png)

## Про проект

Те, кто когда\-то интересовался темой Big Data, наверняка слышал или работал с таким проектом, как [Apache Hadoop](https://hadoop.apache.org/)\. Если очень коротко, то Hadoop – фреймворк, который можно использовать в качестве основы для построения систем Big Data и работы с ними\. 

Hadoop состоит из четырёх основных модулей, каждый из которых выполняет определённую задачу, необходимую для системы аналитики больших данных:

* Hadoop Common
* MapReduce 
* Распределённая файловая система Hadoop HDFS \(Hadoop Distributed File System\)
* YARN

Впрочем, материалов для ознакомления с ним полно в интернете\.

## О проверке

Как показано в [документации](https://pvs-studio.ru/ru/docs/manual/6703/), PVS\-Studio может интегрироваться в проект различными способами:

* использование maven плагина; 
* использование gradle плагина;
* использование IntellJ IDEA;
* использование анализатора напрямую\. 

Hadoop построен на основе сборочной системы maven, поэтому сложностей с проверкой не возникло\.

Интегрировав скрипт из документации и немного скорректировав один из pom\.xml \(в зависимостях были модули, которых не было\), анализ пошел\!

После анализа, выбрав наиболее интересные предупреждения, я обратил внимание, что у меня набралось одинаковое количество предупреждений как в production коде, так и в тестах\. Обычно я не рассматриваю срабатывания анализатора, которые приходятся на тесты\. Но, разделив их, я не смог пропустить предупреждения из категории 'тесты' мимо своего внимания\. "А почему бы и нет?" – подумал я, ведь баги в тестах также имеют свои последствия\. Они могут приводить к некорректному, либо частичному тестированию, а то и вовсе к бессмыслице \(лишь для галочки, чтобы были всегда зелененькие\)\. 

Итак, собрав наиболее интересные предупреждения, разделив их по принадлежности к коду \(production, test\) и по четырем основным модулям Hadoop, предлагаю вашему вниманию разбор срабатываний анализатора\.

## Production code

### Hadoop Common

[V6033](https://pvs-studio.ru/ru/docs/warnings/v6033/) An item with the same key 'KDC\_BIND\_ADDRESS' has already been added\. MiniKdc\.java\(163\), MiniKdc\.java\(162\)

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v6072/) Two similar code fragments were found\. Perhaps, this is a typo and 'localFiles' variable should be used instead of 'localArchives'\. LocalDistributedCacheManager\.java\(183\), LocalDistributedCacheManager\.java\(178\), LocalDistributedCacheManager\.java\(176\), LocalDistributedCacheManager\.java\(181\)

```cpp
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 делает иногда очень интересные находки\. Суть диагностики заключается в поиске однотипных фрагментов кода, которые были получены путем copy\-paste и заменой одной\-двух переменных, но при этом некоторые переменные так и "недоизменили"\.

Вышеприведенный код это демонстрирует\. В первом блоке производятся действия с переменной _localArchives_, в следующем однотипном фрагменте \- с _localFiles_\. И если вы добросовестно изучите этот код, а не бегло пробежитесь, как это частенько бывает при code review, то заметите место, где забыли заменить переменную _localArchives_\.

Такая оплошность может привести к следующему сценарию:

* Предположим, у нас есть _localArchives_ \(size \= 4\) и _localFiles_ \(size \= 2\);
* При создании массива _localFiles\.toArray\(new String\[localArchives\.size\(\)\]\)_ у нас получится такое, что последние 2 элемента будут _null_ \(\["pathToFile1", "pathToFile2", null, null\]\);
* После чего _org\.apache\.hadoop\.util\.StringUtils\.arrayToString_ нам вернет строковое представление нашего массива, в котором последние имена файлов будут представлены как "null" \("pathToFile1, pathToFile2, null, null"_\)_;
* Все это будет передано дальше, и кто знает, какие там есть проверки на такие случаи \=\)\.

[V6007](https://pvs-studio.ru/ru/docs/warnings/v6007/) Expression 'children\.size\(\) \> 0' is always true\. Queue\.java\(347\)

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

Из\-за того, что проверка количества элементов на 0 производится отдельно, дальнейшая проверка _children\.size\(\) \> 0_ будет всегда давать истину\.

### HDFS

[V6001](https://pvs-studio.ru/ru/docs/warnings/v6001/) There are identical sub\-expressions 'this\.bucketSize' to the left and to the right of the '%' operator\. RollingWindow\.java\(79\)

```cpp
  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_\), исключение так и не будет выброшено\.  

### YARN

[V6067](https://pvs-studio.ru/ru/docs/warnings/v6067/) Two or more case\-branches perform the same actions\. TimelineEntityV2Converter\.java\(386\), TimelineEntityV2Converter\.java\(389\)

```cpp
 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;
      }
    }
    ....
  }
  ....
}
```

В двух _case_ ветвях одни и те же фрагменты кода\. Такое встречается сплошь и рядом\! В преобладающем количестве случаев это не является настоящей ошибкой, а лишь поводом задуматься о рефакторинге _switch_\. Но не для рассматриваемого случая\. В повторяющихся фрагментах кода устанавливается значение переменной _preemptedVcoreSeconds_\. Если вы обратите внимание на названия всех переменных и констант, то можно прийти к выводу, что в случае _metric\.getId\(\) \=\= APP\_MEM\_PREEMPT\_METRICS_ должно устанавливаться значение переменной _preemptedMemorySeconds_, а не _preemptedVcoreSeconds_\. В связи с этим, _preemptedMemorySeconds_ после выполнения оператора 'switch' будет оставаться всегда 0, а значение _preemptedVcoreSeconds_ может быть неверным\.

[V6046](https://pvs-studio.ru/ru/docs/warnings/v6046/) Incorrect format\. A different number of format items is expected\. Arguments not used: 2\. AbstractSchedulerPlanFollower\.java\(186\)

```cpp
@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_ при логировании\. Тут или слишком много скопировали, или не доработали форматную строку\. Но я все же склоняюсь к старому доброму и, иногда приносящему медвежью услугу, copy\-paste'у\.

## Test code

### Hadoop Common

[V6072](https://pvs-studio.ru/ru/docs/warnings/v6072/) Two similar code fragments were found\. Perhaps, this is a typo and 'allSecretsB' variable should be used instead of 'allSecretsA'\. TestZKSignerSecretProvider\.java\(316\), TestZKSignerSecretProvider\.java\(309\), TestZKSignerSecretProvider\.java\(306\), TestZKSignerSecretProvider\.java\(313\)

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v6043/) Consider inspecting the 'for' operator\. Initial and final values of the iterator are the same\. TestTFile\.java\(235\)

```cpp
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 < start_ сразу же выдаст нам false, что и приведет к такому поведению\. Я пробежался по файлику с тестами и пришел к выводу, что требовалось написать в условии цикла _i < \(start \+ n\)_\.

### MapReduce

[V6007](https://pvs-studio.ru/ru/docs/warnings/v6007/) Expression 'byteAm < 0' is always false\. DataWriter\.java\(322\)

```cpp
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_ всегда ложь\. Чтобы разобраться, давайте поднимемся по коду выше\. Если выполнение теста дойдет до операции _byteAm \-\= headerLen_, то это означает, что будет _byteAm \>\= headerLen_\. Отсюда, после выполнения вычитания, значение _byteAm_ никогда не будет отрицательным\. Что и требовалось доказать\. 

### HDFS

[V6072](https://pvs-studio.ru/ru/docs/warnings/v6072/) Two similar code fragments were found\. Perhaps, this is a typo and 'normalFile' variable should be used instead of 'normalDir'\. TestWebHDFS\.java\(625\), TestWebHDFS\.java\(615\), TestWebHDFS\.java\(614\), TestWebHDFS\.java\(624\)

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v6027/) Variables are initialized through the call to the same function\. It's probably an error or un\-optimized code\. TestDFSAdmin\.java\(883\), TestDFSAdmin\.java\(879\)

```cpp
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_ проинициализированы одинаковыми значениями\. Часто так и должно быть, но не в данной ситуации\. Названия переменных тут длинные и похожи друг на друга, так что я не удивлен, что при copy\-paste не было соответствующих модификаций\. Для исправления ситуации, при инициализации переменной _highestPriorityLowRedundancyECBlocksStr_ нужно использовать входной параметр _highestPriorityLowRedundancyECBlocks_\. Помимо этого, скорее всего, нужно еще поправить форматную строку\.

[V6019](https://pvs-studio.ru/ru/docs/warnings/v6019/) Unreachable code detected\. It is possible that an error is present\. TestReplaceDatanodeFailureReplication\.java\(222\)

```cpp
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\+\+\) \{\.\.\.\.\}_ будет выполняться не более одной итерации\. Давайте выясним, почему же\. Итак, на первой итерации мы связываем поток с файлом, соответствующим _slowwriters\[0\]_, для дальнейшего считывания\. Через цикл _for \(int j \= 0, x;; j\+\+\)_ побайтово считываем содержимое файла, где:

* если прочли что\-то адекватное, то через _assertEquals_ сравниваем прочитанный байт с текущим значением счетчика _j_ \(в случае неудачной проверки мы вылетаем из теста с fail'ом\);
* если файл прошел проверку, и мы дошли до конца файла \(прочли \-1\), то мы выходим из метода\. 

Поэтому, что бы не произошло при проверке _slowwriters\[0\]_, дело до проверки следующих элементов так и не дойдет\. Скорее всего, вместо _return_ необходимо использовать _break_\.

### YARN

[V6019](https://pvs-studio.ru/ru/docs/warnings/v6019/) Unreachable code detected\. It is possible that an error is present\. TestNodeManager\.java\(176\)

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

В данной ситуации stacktrace так и не будет напечатан в случае возникновения исключения, так как метод _Assert\.fail_ прервет выполнение теста\. Если здесь достаточно сообщения, что исключение поймано, то, чтобы не путаться, печать stacktrace'a нужно удалить\. Если же печать необходима, то нужно всего\-навсего поменять их местами\.

Таких мест много:

* V6019 Unreachable code detected\. It is possible that an error is present\. TestResourceTrackerService\.java\(928\)
* V6019 Unreachable code detected\. It is possible that an error is present\. TestResourceTrackerService\.java\(737\)
* V6019 Unreachable code detected\. It is possible that an error is present\. TestResourceTrackerService\.java\(685\)
* \.\.\.\.

[V6072](https://pvs-studio.ru/ru/docs/warnings/v6072/) Two similar code fragments were found\. Perhaps, this is a typo and 'publicCache' variable should be used instead of 'usercache'\. TestResourceLocalizationService\.java\(315\), TestResourceLocalizationService\.java\(309\), TestResourceLocalizationService\.java\(307\), TestResourceLocalizationService\.java\(313\)

```cpp
@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_\.  

## Заключение

При разработке пишутся сотни тысяч строк кода\. Если production код стремятся поддерживать в чистоте от багов, дефектов и недочетов \(разработчик сам тестирует свой код, проводят code review и многое другое\), то тесты в этом явно уступают\. Дефекты в тестах спокойно могут скрываться за "зеленой галочкой"\. И как вы поняли из сегодняшнего разбора предупреждений, успешно пройденный тест далеко не всегда является гарантированной проверкой\.

При проверке кодовой базы Apache Hadoop статический анализ продемонстрировал свою необходимость не только в коде, который идет в production, но и в тестах, которые также играют немаловажную роль при разработке\.

Так что если вам не все равно на качество вашего кода и тестовой базы, то рекомендую вам посмотреть в сторону статического анализа\. И первым претендентом для испытания предлагаю попробовать [PVS\-Studio](https://pvs-studio.ru/ru/pvs-studio/download/)\.