Geekly articles weekly

Kualitas Kode Apache Hadoop: Produksi VS Tes

Gambar 1

Untuk mendapatkan kode produksi berkualitas tinggi, tidak cukup hanya memastikan cakupan maksimum dengan pengujian. Tidak diragukan lagi, hasil yang luar biasa membutuhkan kode proyek utama dan tes untuk bekerja secara efisien bersama. Oleh karena itu, tes harus diperhatikan sebanyak kode utama. Tes yang layak adalah faktor kunci keberhasilan, karena akan menangkap regresi dalam produksi. Mari kita lihat peringatan analisa statis PVS-Studio untuk melihat pentingnya fakta bahwa kesalahan dalam tes tidak lebih buruk daripada yang ada di produksi. Fokus hari ini: Apache Hadoop.

Tentang proyek


Mereka yang sebelumnya tertarik dengan Big Data mungkin pernah mendengar atau bahkan bekerja dengan proyek Apache Hadoop . Singkatnya, Hadoop adalah kerangka kerja yang dapat digunakan sebagai dasar untuk membangun sistem Big Data dan bekerja dengannya.

Hadoop terdiri dari empat modul utama, masing-masingnya melakukan tugas khusus yang diperlukan untuk sistem analisis data besar:

  • Hadoop biasa
  • Kurangi peta
  • Sistem File Terdistribusi Hadoop
  • Benang

Bagaimanapun, ada banyak materi tentang itu di Internet.

Tentang cek


Seperti yang ditunjukkan dalam dokumentasi, PVS-Studio dapat diintegrasikan dalam proyek dengan berbagai cara:

  • Menggunakan plugin maven;
  • Menggunakan plugin gradle;
  • Menggunakan IntellJ IDEA gradle;
  • Langsung menggunakan analisa.

Hadoop didasarkan pada sistem pembangunan pakar, oleh karena itu, tidak ada hambatan dengan cek.

Setelah saya mengintegrasikan skrip dari dokumentasi dan mengedit salah satu file pom.xml (ada modul dalam dependensi yang tidak tersedia), analisis dimulai!

Setelah analisis selesai, saya memilih peringatan yang paling menarik dan memperhatikan bahwa saya memiliki jumlah peringatan yang sama dalam kode produksi dan dalam pengujian. Biasanya, saya tidak mempertimbangkan peringatan penganalisa, diberikan untuk tes. Tetapi ketika saya membaginya, saya tidak bisa meninggalkan peringatan 'tes' tanpa pengawasan. "Mengapa tidak melihatnya?" - Saya pikir, karena bug dalam tes mungkin juga memiliki konsekuensi yang merugikan. Mereka dapat menyebabkan pengujian yang salah atau sebagian, atau bahkan kesalahan (mereka ada hanya untuk mencentang kotak, bahwa mereka selalu hijau).

Setelah saya memilih peringatan yang paling menarik, saya membaginya dengan kelompok-kelompok berikut: produksi, pengujian dan empat modul Hadoop utama. Dan sekarang saya dengan senang hati memberikan ulasan Anda tentang peringatan penganalisa.

Kode produksi


Hadoop biasa


V6033 Item dengan kunci yang sama 'KDC_BIND_ADDRESS' telah ditambahkan. 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); .... } .... }

Nilai tambah dua kali dalam HashSet adalah cacat yang sangat umum saat memeriksa proyek. Penambahan kedua sebenarnya akan diabaikan. Saya harap duplikasi ini hanyalah sebuah tragedi yang tidak perlu. Bagaimana jika nilai lain dimaksudkan untuk ditambahkan?

Kurangi peta


V6072 Dua fragmen kode serupa ditemukan. Mungkin, ini adalah kesalahan ketik dan variabel 'localFiles' harus digunakan daripada '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()]))); } .... }

Diagnostik V6072 terkadang menghasilkan beberapa temuan menarik. Tujuan diagnostik ini adalah untuk mendeteksi fragmen kode tipe yang sama yang dihasilkan dari salin-tempel dan penggantian satu-dua variabel. Dalam hal ini, beberapa variabel bahkan dibiarkan "tidak berubah".

Kode di atas menunjukkan ini. Di blok pertama variabel localArchives digunakan, di fragmen serupa yang kedua - localFiles . Jika Anda mempelajari kode ini dengan uji tuntas, alih-alih jalankan dengan cepat, seperti yang sering terjadi saat meninjau kode, Anda akan melihat fragmen, di mana penulis lupa mengganti variabel localArchives .

Kesalahan ini dapat menyebabkan skenario berikut:

  • Misalkan, kita memiliki Arsip lokal (ukuran = 4) dan Berkas lokal (ukuran = 2);
  • Saat membuat array localFiles.toArray (String baru [localArchives.size ()])) , 2 elemen terakhir akan menjadi null (["pathToFile1", "pathToFile2", null, null]);
  • Kemudian org.apache.hadoop.util.StringUtils.arrayToString akan mengembalikan representasi string dari array kami, di mana nama file terakhir akan disajikan sebagai "null" ("pathToFile1, pathToFile2, null, null" ) ;
  • Semua ini akan diteruskan lebih lanjut dan hanya Tuhan yang tahu jenis cek apa yang ada untuk kasus seperti itu =).

Ekspresi V6007 'children.size ()> 0' selalu benar. Queue.java (347)

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

Karena fakta bahwa jumlah elemen diperiksa untuk 0 secara terpisah, pemeriksaan selanjutnya pada children.size ()> 0 akan selalu benar.

HDFS


V6001 Ada sub-ekspresi identik 'this.bucketSize' di sebelah kiri dan di sebelah kanan 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); } }

Di sini cacatnya adalah bahwa variabel dibagi dengan sendirinya. Akibatnya, pemeriksaan untuk multiplisitas akan selalu berhasil dan jika mendapat input yang salah ( windowLenMs , numBuckets ), pengecualian tidak akan pernah dilempar.

Benang


V6067 Dua atau lebih cabang kasus melakukan tindakan yang sama. 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; } } .... } .... }

Fragmen kode yang sama di dua cabang case . Itu hanya di semua tempat! Dalam jumlah kasus yang berlaku, ini bukan kesalahan nyata, tetapi hanya alasan untuk berpikir tentang pengembalian sakelar . Tetapi tidak untuk kasus yang dihadapi. Fragmen kode yang diulang menetapkan nilai variabel preemptedVcoreSeconds . Jika Anda melihat dekat pada nama semua variabel dan konstanta, Anda mungkin akan menyimpulkan bahwa jika metric.getId () == APP_MEM_PREEMPT_METRICS nilai harus ditetapkan untuk variabel PreemptedMemorySeconds , bukan untuk preemptedVcoreSeconds . Dalam hal ini, setelah operator 'switch', preemptedMemorySeconds akan selalu tetap 0, sedangkan nilai preemptedVcoreSeconds mungkin salah.

V6046 Format salah. Jumlah item format yang berbeda diharapkan. Argumen tidak digunakan: 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); } .... }

Variabel planQueueName tidak digunakan saat masuk. Dalam hal ini, terlalu banyak disalin atau string format belum selesai. Tapi saya masih menyalahkan copy-paste lama yang bagus, yang dalam beberapa kasus bagus untuk menembak diri sendiri.

Kode uji


Hadoop biasa


V6072 Dua fragmen kode serupa ditemukan. Mungkin, ini adalah kesalahan ketik dan variabel 'allSecretsB' harus digunakan alih-alih '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]); .... }

Dan lagi V6072. Perhatikan baik-baik variabel allSecretsA dan allSecretsB .

V6043 Pertimbangkan untuk memeriksa operator 'untuk'. Nilai awal dan akhir dari iterator adalah sama. 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); }

Tes yang selalu hijau? =). Bagian dari loop, yang merupakan bagian dari tes itu sendiri, tidak akan pernah dieksekusi. Ini disebabkan oleh fakta bahwa nilai penghitung awal dan akhir sama dalam pernyataan for . Akibatnya, kondisi yang saya mulai akan segera menjadi salah, yang mengarah ke perilaku tersebut. Saya berlari melalui file tes dan melompat ke kesimpulan bahwa sebenarnya saya <(mulai + n) harus ditulis dalam kondisi loop.

Kurangi peta


Ekspresi V6007 'byteAm <0' selalu salah. 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; } .... }

Kondisi byteAm <0 selalu salah. Untuk mengetahuinya, mari kita beri kode di atas pandangan lain. Jika eksekusi pengujian mencapai operasi byteAm - = headerLen , itu berarti byteAm> = headerLen. Dari sini, setelah dikurangi, nilai byteAm tidak akan pernah negatif. Itu yang harus kita buktikan.

HDFS


V6072 Dua fragmen kode serupa ditemukan. Mungkin, ini adalah kesalahan ketik dan variabel 'normalFile' harus digunakan daripada '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 " + ....); }

Percaya atau tidak, itu V6072 lagi! Cukup ikuti variabel normalDir dan normalFile.

V6027 Variabel diinisialisasi melalui panggilan ke fungsi yang sama. Mungkin salah atau kode tidak dioptimalkan. 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); .... }

Dalam fragmen ini, variabel tertinggiPriorityLowRedundancyReplicatedBlocksStr dan tertinggiPriorityLowRedundancyECBlocksStr diinisialisasi dengan nilai yang sama. Seringkali memang seharusnya begitu, tetapi ini tidak terjadi. Nama-nama variabel panjang dan serupa, jadi tidak mengherankan bahwa fragmen yang disalin tidak diubah dengan cara apa pun. Untuk memperbaikinya, ketika menginisialisasi variabel tertinggiPriorityLowRedundancyECBlocksStr, penulis harus menggunakan parameter input tertinggiPriorityLowRedundancyECBlocks . Selain itu, kemungkinan besar, mereka masih perlu memperbaiki garis format.

V6019 Kode tidak terjangkau terdeteksi. Mungkin saja ada kesalahan. 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); } } }

Penganalisa mengeluh bahwa penghitung i ++ dalam loop tidak dapat diubah. Yang berarti bahwa dalam loop for (int i = 0; i <slowwriters.length; i ++) {....} tidak lebih dari satu iterasi yang akan dieksekusi. Mari kita cari tahu alasannya. Jadi, dalam iterasi pertama, kami menautkan utas dengan file yang sesuai dengan slowwriters [0] untuk dibaca lebih lanjut. Selanjutnya, kita membaca konten file melalui loop untuk (int j = 0, x ;; j ++):

  • jika kita membaca sesuatu yang relevan, kita membandingkan byte baca dengan nilai saat ini dari penghitung j melalui assertEquals (jika pemeriksaan tidak berhasil, kita gagal dalam pengujian);
  • jika file diperiksa dengan sukses dan kita sampai ke akhir file (kita baca -1), metode keluar.

Oleh karena itu, apa pun yang terjadi selama pemeriksaan slowwriter [0] , ia tidak akan memeriksa elemen berikutnya. Kemungkinan besar, break harus digunakan alih-alih kembali.

Benang


V6019 Kode tidak terjangkau terdeteksi. Mungkin saja ada kesalahan. TestNodeManager.java (176)

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

Dalam situasi ini, metode Assert.fail akan mengganggu tes dan stacktrace tidak akan dicetak jika ada pengecualian. Jika pesan tentang pengecualian yang ditangkap sudah cukup di sini, lebih baik untuk menghapus pencetakan stacktrace untuk menghindari kebingungan. Jika perlu mencetak, Anda hanya perlu menukar mereka.

Banyak fragmen serupa telah ditemukan:

  • V6019 Kode tidak terjangkau terdeteksi. Mungkin saja ada kesalahan. TestResourceTrackerService.java (928)
  • V6019 Kode tidak terjangkau terdeteksi. Mungkin saja ada kesalahan. TestResourceTrackerService.java (737)
  • V6019 Kode tidak terjangkau terdeteksi. Mungkin saja ada kesalahan. TestResourceTrackerService.java (685)
  • ....

V6072 Dua fragmen kode serupa ditemukan. Mungkin, ini adalah kesalahan ketik dan variabel 'publicCache' harus digunakan daripada '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), ....); .... } .... }

Dan akhirnya, V6072 lagi =). Variabel untuk melihat fragmen yang mencurigakan: usercache dan publicCache .

Kesimpulan


Ratusan ribu baris kode ditulis dalam pengembangan. Kode produksi biasanya tetap bersih dari bug, cacat dan cacat (pengembang menguji kode mereka, kode ditinjau dan seterusnya). Tes pasti lebih rendah dalam hal ini. Cacat dalam tes dapat dengan mudah bersembunyi di balik "centang hijau". Seperti yang mungkin Anda dapatkan dari ulasan peringatan hari ini, tes hijau tidak selalu berhasil.

Kali ini, ketika memeriksa basis kode Apache Hadoop, analisis statis ternyata sangat dibutuhkan baik dalam kode produksi dan tes yang juga memainkan peran penting dalam pengembangan.

Jadi, jika Anda peduli dengan kualitas kode dan pengujian Anda, saya sarankan Anda mengarahkan analisis statis. Biarkan PVS-Studio menjadi pesaing pertama dalam usaha ini.

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

More articles:


All Articles