Максим Стефанов

18 Дек 2019

Теги:

#Java

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

18 Дек 2019

Автор: Максим Стефанов

Про проект
О проверке
Production code
Test code
Заключение

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

Про проект

Те, кто когда-то интересовался темой Big Data, наверняка слышал или работал с таким проектом, как Apache Hadoop. Если очень коротко, то Hadoop – фреймворк, который можно использовать в качестве основы для построения систем Big Data и работы с ними.

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

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

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

О проверке

Как показано в документации, PVS-Studio может интегрироваться в проект различными способами:

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

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

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

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

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

Production code

Hadoop Common

V6033 An item with the same key 'KDC_BIND_ADDRESS' has already been added. 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 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)

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 Expression 'children.size() > 0' is always true. Queue.java(347)

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

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

HDFS

V6001 There are identical sub-expressions 'this.bucketSize' to the left and to the right of the '%' operator. 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), исключение так и не будет выброшено.

YARN

V6067 Two or more case-branches perform the same 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;
      }
    }
    ....
  }
  ....
}

В двух case ветвях одни и те же фрагменты кода. Такое встречается сплошь и рядом! В преобладающем количестве случаев это не является настоящей ошибкой, а лишь поводом задуматься о рефакторинге switch. Но не для рассматриваемого случая. В повторяющихся фрагментах кода устанавливается значение переменной preemptedVcoreSeconds. Если вы обратите внимание на названия всех переменных и констант, то можно прийти к выводу, что в случае metric.getId() == APP_MEM_PREEMPT_METRICS должно устанавливаться значение переменной preemptedMemorySeconds, а не preemptedVcoreSeconds. В связи с этим, preemptedMemorySeconds после выполнения оператора 'switch' будет оставаться всегда 0, а значение preemptedVcoreSeconds может быть неверным.

V6046 Incorrect format. A different number of format items is expected. Arguments not used: 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 при логировании. Тут или слишком много скопировали, или не доработали форматную строку. Но я все же склоняюсь к старому доброму и, иногда приносящему медвежью услугу, copy-paste'у.

Test code

Hadoop Common

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)

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 Consider inspecting the 'for' operator. Initial and final values of the iterator are the same. 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 < start сразу же выдаст нам false, что и приведет к такому поведению. Я пробежался по файлику с тестами и пришел к выводу, что требовалось написать в условии цикла i < (start + n).

MapReduce

V6007 Expression 'byteAm < 0' is always 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 всегда ложь. Чтобы разобраться, давайте поднимемся по коду выше. Если выполнение теста дойдет до операции byteAm -= headerLen, то это означает, что будет byteAm >= headerLen. Отсюда, после выполнения вычитания, значение byteAm никогда не будет отрицательным. Что и требовалось доказать.

HDFS

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)

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

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 Unreachable code detected. It is possible that an error is present. 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++) {....} будет выполняться не более одной итерации. Давайте выясним, почему же. Итак, на первой итерации мы связываем поток с файлом, соответствующим slowwriters[0], для дальнейшего считывания. Через цикл for (int j = 0, x;; j++) побайтово считываем содержимое файла, где:

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

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

YARN

V6019 Unreachable code detected. It is possible that an error is present. TestNodeManager.java(176)

@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 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)

@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.

#Java