اباتشي Hadoop رمز الجودة: الإنتاج مقابل الاختبار

من أجل الحصول على كود إنتاج عالي الجودة ، لا يكفي فقط ضمان أقصى قدر من التغطية للاختبارات. لا شك ، النتائج الكبيرة تتطلب رمز المشروع الرئيسي والاختبارات للعمل معا بكفاءة. لذلك ، يجب إيلاء الاختبارات الكثير من الاهتمام مثل الرمز الرئيسي. يعد الاختبار اللائق أحد عوامل النجاح الرئيسية ، حيث سيؤدي إلى تراجع الإنتاج. دعونا نلقي نظرة على تحذيرات محلل ثابت PVS-Studio لمعرفة أهمية حقيقة أن الأخطاء في الاختبارات ليست أسوأ من تلك التي في الإنتاج. التركيز اليوم: اباتشي Hadoop.

عن المشروع

أولئك الذين كانوا مهتمين في السابق بالبيانات الكبيرة ربما سمعوا أو عملوا مع مشروع Apache Hadoop . باختصار ، Hadoop هو إطار يمكن استخدامه كأساس لبناء أنظمة البيانات الكبيرة والعمل معها.

يتكون Hadoop من أربع وحدات رئيسية ، كل منها يؤدي مهمة محددة مطلوبة لنظام تحليل البيانات الكبيرة:

• Hadoop المشتركة
• مابريديوس
• Hadoop نظام الملفات الموزعة
• غزل

على أي حال ، هناك الكثير من المواد حول هذا الموضوع على الإنترنت.

عن الشيك

كما هو موضح في الوثائق ، يمكن دمج PVS-Studio في المشروع بعدة طرق:

• باستخدام البرنامج المساعد مخضرم.
• استخدام المكوّن الإضافي ؛
• باستخدام مهد IntellJ IDEA ؛
• مباشرة باستخدام محلل.

يستند Hadoop على نظام الإنشاء المخضرم ، وبالتالي ، لم تكن هناك عقبات مع الاختيار.

بعد دمج البرنامج النصي من الوثائق وتحرير أحد ملفات pom.xml (كانت هناك وحدات في التبعيات التي لم تكن متوفرة) ، بدأ التحليل!

بعد اكتمال التحليل ، اخترت التحذيرات الأكثر إثارة للاهتمام ولاحظت أنني تلقيت نفس عدد التحذيرات في كود الإنتاج وفي الاختبارات. عادة ، أنا لا أعتبر تحذيرات المحلل ، المقدمة للاختبارات. لكن عندما قسمتها ، لم أستطع ترك تحذيرات "الاختبارات" دون مراقبة. "لماذا لا نلقي نظرة عليهم؟" - اعتقدت ، لأن الأخطاء في الاختبارات قد يكون لها أيضًا عواقب وخيمة. يمكن أن تؤدي إلى اختبار غير صحيح أو جزئي ، أو حتى mishmash (أنها موجودة فقط للتحقق من مربع ، وأنها دائما خضراء).

بعد تحديد التحذيرات الأكثر إثارة للاهتمام ، قسمتها على المجموعات التالية: الإنتاج والاختبار ووحدات 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 عيبًا شائعًا جدًا عند التحقق من المشروعات. سيتم تجاهل الإضافة الثانية بالفعل. آمل أن تكون هذه الازدواجية مجرد مأساة لا داعي لها. ماذا لو كان من المفترض أن تضاف قيمة أخرى؟

مابريديوس

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 .

يمكن أن يؤدي هذا gaffe إلى السيناريو التالي:

• لنفترض أن لدينا أرشيفية محلية (حجم = 4) وملف محلي (حجم = 2) ؛
• عند إنشاء الصفيف localFiles.toArray (سلسلة جديدة [localArchives.size ()]) ، سيكون العنصران الأخيران خاليان (["pathToFile1" ، "pathToFile2" ، null ، null]) ؛
• ثم org.apache.hadoop.util.StringUtils.arrayToString سيعود تمثيل سلسلة الصفيف لدينا ، حيث سيتم تقديم أسماء الملفات الأخيرة كـ "خالية" ("pathToFile1 ، pathToFile2 ، null ، null" ) ؛
• سيتم تمرير كل هذا إلى أبعد من ذلك ، والله وحده يعلم نوع الشيكات الموجودة لمثل هذه الحالات =).

تعبير V6007 'children.size ()> 0' صحيح دائمًا. Queue.java (347)

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

نظرًا لحقيقة أن عدد العناصر محددًا على 0 بشكل منفصل ، فسيظل التحقق من children.size ()> 0 صحيحًا دائمًا.

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

شظايا نفس الرمز في فرعين للحالة . انها فقط في كل مكان! في العدد السائد من الحالات ، هذا ليس خطأ حقيقيًا ، ولكن مجرد سبب للتفكير في إصلاح التبديل . ولكن ليس للقضية في متناول اليد. تقوم أجزاء التعليمات البرمجية المتكررة بتعيين قيمة المتغير preemptedVcoreSeconds . إذا نظرت عن كثب إلى أسماء جميع المتغيرات والثوابت ، فمن المحتمل أن تستنتج أنه في حالة إذا كان metric.getId () == APP_MEM_PREEMPT_METRICS ، فيجب تعيين القيمة لمتغير preemptedMemorySeconds ، وليس لمتغيرات preemptedVcoreSeconds . في هذا الصدد ، بعد عامل تشغيل "التبديل" ، ستبقى 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 . نتيجة لذلك ، فإن الشرط الذي سأبدأ به سيبدأ على الفور ، مما يؤدي إلى مثل هذا السلوك. ركضت من خلال ملف الاختبار وقفزت إلى استنتاج مفاده أنه في الواقع يجب أن أكون مكتوبًا (في البداية + n) في حالة الحلقة.

مابريديوس

تعبير V6007 'byteAm <0' غير صحيح دائمًا. 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; } .... } 

شرط البايت <0 غير صحيح دائمًا. لمعرفة ذلك ، دعونا نعطي الكود أعلاه نظرة أخرى. إذا وصل تنفيذ الاختبار إلى العملية 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 يتم تهيئة المتغيرات من خلال الدعوة إلى نفس الوظيفة. ربما يكون خطأ أو رمز un-optimized. 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); .... } 

في هذه الشريحة ، تتم تهيئة المتغيرات topPriorityLowRedundancyReplicatedBlocksStr و topPriorityLowRedundancyECBlocksStr بنفس القيم. في كثير من الأحيان يجب أن يكون الأمر كذلك ، ولكن هذا ليس هو الحال. أسماء المتغيرات طويلة ومتشابهة ، لذلك ليس من المستغرب أن جزءًا من لصق النسخ لم يتغير بأي طريقة. لإصلاحها ، عند تهيئة متغير higherPriorityLowRedundancyECBlocksStr ، يتعين على المؤلف استخدام معلمة الإدخال higherPriorityLowRedundancyECBlocks . بالإضافة إلى ذلك ، على الأرجح ، لا يزال يتعين عليهم تصحيح خط التنسيق.

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

• إذا قرأنا شيئًا ما ذا صلة ، فسنقارن قراءة البايت بالقيمة الحالية للعداد j على الرغم من assertEquals (إذا لم يكن الاختبار ناجحًا ، فإننا نفشل في الاختبار) ؛
• إذا تم التحقق من الملف بنجاح ووصلنا إلى نهاية الملف (نقرأ -1) ، فستخرج الطريقة.

لذلك ، أيا كان ما يحدث أثناء التحقق من كتابات البطيئة [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 أول منافس في هذا المشروع.