From 28f8b7d1bb6c85b709ad0b1fcd9dc0a9f481ecca Mon Sep 17 00:00:00 2001 From: Murtaza Hassan Date: Thu, 22 Aug 2019 14:22:26 +0200 Subject: [PATCH 1/4] HBASE-22766 Added unit tests for ResultStatsUtil to have 100 percent code coverage and removed two methods from ResulStatsUtil --- .../hadoop/hbase/client/ResultStatsUtil.java | 42 ++--------- .../hbase/client/TestResultStatsUtil.java | 72 +++++++++++++++++++ 2 files changed, 76 insertions(+), 38 deletions(-) create mode 100644 hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestResultStatsUtil.java diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ResultStatsUtil.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ResultStatsUtil.java index af45325df8df..d17219cfd2b9 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ResultStatsUtil.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ResultStatsUtil.java @@ -17,12 +17,11 @@ */ package org.apache.hadoop.hbase.client; -import org.apache.hadoop.hbase.HRegionLocation; import org.apache.hadoop.hbase.ServerName; import org.apache.yetus.audience.InterfaceAudience; /** - * A {@link Result} with some statistics about the server/region status + * statistics update about a server/region */ @InterfaceAudience.Private public final class ResultStatsUtil { @@ -32,48 +31,15 @@ private ResultStatsUtil() { } /** - * Update the stats for the specified region if the result is an instance of {@link - * ResultStatsUtil} - * - * @param r object that contains the result and possibly the statistics about the region - * @param serverStats stats tracker to update from the result + * @param tracker tracker to update * @param server server from which the result was obtained - * @param regionName full region name for the stats. - * @return the underlying {@link Result} if the passed result is an {@link - * ResultStatsUtil} or just returns the result; + * @param regionName full region name for the stats + * @param stats stats to update for the specified region */ - public static T updateStats(T r, ServerStatisticTracker serverStats, - ServerName server, byte[] regionName) { - if (!(r instanceof Result)) { - return r; - } - Result result = (Result) r; - // early exit if there are no stats to collect - RegionLoadStats stats = result.getStats(); - if(stats == null){ - return r; - } - - updateStats(serverStats, server, regionName, stats); - return r; - } - public static void updateStats(StatisticTrackable tracker, ServerName server, byte[] regionName, RegionLoadStats stats) { if (regionName != null && stats != null && tracker != null) { tracker.updateRegionStats(server, regionName, stats); } } - - public static T updateStats(T r, ServerStatisticTracker stats, - HRegionLocation regionLocation) { - byte[] regionName = null; - ServerName server = null; - if (regionLocation != null) { - server = regionLocation.getServerName(); - regionName = regionLocation.getRegion().getRegionName(); - } - - return updateStats(r, stats, server, regionName); - } } diff --git a/hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestResultStatsUtil.java b/hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestResultStatsUtil.java new file mode 100644 index 000000000000..806e163c31a9 --- /dev/null +++ b/hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestResultStatsUtil.java @@ -0,0 +1,72 @@ +package org.apache.hadoop.hbase.client; + +import org.apache.hadoop.hbase.HBaseClassTestRule; +import org.apache.hadoop.hbase.ServerName; +import org.apache.hadoop.hbase.client.backoff.ServerStatistics; +import org.apache.hadoop.hbase.testclassification.MiscTests; +import org.apache.hadoop.hbase.testclassification.RegionServerTests; +import org.junit.Assert; +import org.junit.ClassRule; +import org.junit.Test; +import org.junit.experimental.categories.Category; + +@Category({MiscTests.class, RegionServerTests.class}) +public class TestResultStatsUtil { + @ClassRule + public static final HBaseClassTestRule CLASS_RULE = + HBaseClassTestRule.forClass(TestGet.class); + + private static final RegionLoadStats regionLoadStats = new RegionLoadStats(100, + 10,90); + private static final byte[] regionName = {80}; + private static final ServerName server = ServerName.parseServerName("3.1.yg.n,50,1"); + + @Test + public void testUpdateStats() { + // Create a tracker + ServerStatisticTracker serverStatisticTracker = new ServerStatisticTracker(); + + // Pass in the tracker for update + ResultStatsUtil.updateStats(serverStatisticTracker, server, regionName, regionLoadStats); + + // Check that the tracker was updated as expected + ServerStatistics stats = serverStatisticTracker.getStats(server); + + Assert.assertEquals(stats.getStatsForRegion(regionName).getMemStoreLoadPercent(), + regionLoadStats.memstoreLoad); + Assert.assertEquals(stats.getStatsForRegion(regionName).getCompactionPressure(), + regionLoadStats.compactionPressure); + Assert.assertEquals(stats.getStatsForRegion(regionName).getHeapOccupancyPercent(), + regionLoadStats.heapOccupancy); + } + + @Test + public void testUpdateStatsRegionNameNull() { + ServerStatisticTracker serverStatisticTracker = new ServerStatisticTracker(); + + ResultStatsUtil.updateStats(serverStatisticTracker, server, null, regionLoadStats); + + ServerStatistics stats = serverStatisticTracker.getStats(server); + Assert.assertEquals(stats,null); + } + + @Test + public void testUpdateStatsStatsNull() { + ServerStatisticTracker serverStatisticTracker = new ServerStatisticTracker(); + + ResultStatsUtil.updateStats(serverStatisticTracker, server, regionName, null); + + ServerStatistics stats = serverStatisticTracker.getStats(server); + Assert.assertEquals(stats,null); + } + + @Test + public void testUpdateStatsTrackerNull() { + ServerStatisticTracker serverStatisticTracker = new ServerStatisticTracker(); + + ResultStatsUtil.updateStats(null, server, regionName, regionLoadStats); + + ServerStatistics stats = serverStatisticTracker.getStats(server); + Assert.assertEquals(stats,null); + } +} \ No newline at end of file From 8f82baa9b258154f5ba630d3a489911fa01dfdb2 Mon Sep 17 00:00:00 2001 From: Murtaza Hassan Date: Thu, 22 Aug 2019 14:25:22 +0200 Subject: [PATCH 2/4] HBASE-22766 improved formatting of code --- .../java/org/apache/hadoop/hbase/client/ResultStatsUtil.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ResultStatsUtil.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ResultStatsUtil.java index d17219cfd2b9..5410c9922135 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ResultStatsUtil.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ResultStatsUtil.java @@ -42,4 +42,4 @@ public static void updateStats(StatisticTrackable tracker, ServerName server, by tracker.updateRegionStats(server, regionName, stats); } } -} +} \ No newline at end of file From 8a127f2b1d6b92b9bdae14df45819e1696993509 Mon Sep 17 00:00:00 2001 From: Murtaza Hassan Date: Thu, 22 Aug 2019 17:44:41 +0200 Subject: [PATCH 3/4] HBASE-22766 Made changes according to feedback --- .../hbase/client/TestResultStatsUtil.java | 25 ++++++++++++++++--- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestResultStatsUtil.java b/hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestResultStatsUtil.java index 806e163c31a9..526317ba3532 100644 --- a/hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestResultStatsUtil.java +++ b/hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestResultStatsUtil.java @@ -1,20 +1,37 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ package org.apache.hadoop.hbase.client; import org.apache.hadoop.hbase.HBaseClassTestRule; import org.apache.hadoop.hbase.ServerName; import org.apache.hadoop.hbase.client.backoff.ServerStatistics; -import org.apache.hadoop.hbase.testclassification.MiscTests; -import org.apache.hadoop.hbase.testclassification.RegionServerTests; +import org.apache.hadoop.hbase.testclassification.ClientTests; +import org.apache.hadoop.hbase.testclassification.SmallTests; import org.junit.Assert; import org.junit.ClassRule; import org.junit.Test; import org.junit.experimental.categories.Category; -@Category({MiscTests.class, RegionServerTests.class}) +@Category({ClientTests.class, SmallTests.class}) public class TestResultStatsUtil { @ClassRule public static final HBaseClassTestRule CLASS_RULE = - HBaseClassTestRule.forClass(TestGet.class); + HBaseClassTestRule.forClass(TestResultStatsUtil.class); private static final RegionLoadStats regionLoadStats = new RegionLoadStats(100, 10,90); From 45fa15503e531d092869d0d1301b58d06674dea1 Mon Sep 17 00:00:00 2001 From: Jan Hentschel Date: Thu, 29 Aug 2019 20:25:15 +0200 Subject: [PATCH 4/4] HBASE-22766 Solved review comments --- .../hadoop/hbase/client/ResultStatsUtil.java | 11 +++++---- .../hbase/client/TestResultStatsUtil.java | 24 ++++++++++--------- 2 files changed, 19 insertions(+), 16 deletions(-) diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ResultStatsUtil.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ResultStatsUtil.java index 5410c9922135..793d10875cb3 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ResultStatsUtil.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ResultStatsUtil.java @@ -21,20 +21,21 @@ import org.apache.yetus.audience.InterfaceAudience; /** - * statistics update about a server/region + * Statistics update about a server/region */ @InterfaceAudience.Private public final class ResultStatsUtil { - private ResultStatsUtil() { //private ctor for util class } /** + * Update the statistics for the specified region. + * * @param tracker tracker to update * @param server server from which the result was obtained - * @param regionName full region name for the stats - * @param stats stats to update for the specified region + * @param regionName full region name for the statistics + * @param stats statistics to update for the specified region */ public static void updateStats(StatisticTrackable tracker, ServerName server, byte[] regionName, RegionLoadStats stats) { @@ -42,4 +43,4 @@ public static void updateStats(StatisticTrackable tracker, ServerName server, by tracker.updateRegionStats(server, regionName, stats); } } -} \ No newline at end of file +} diff --git a/hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestResultStatsUtil.java b/hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestResultStatsUtil.java index 526317ba3532..5b591030c966 100644 --- a/hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestResultStatsUtil.java +++ b/hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestResultStatsUtil.java @@ -17,12 +17,14 @@ */ package org.apache.hadoop.hbase.client; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; + import org.apache.hadoop.hbase.HBaseClassTestRule; import org.apache.hadoop.hbase.ServerName; import org.apache.hadoop.hbase.client.backoff.ServerStatistics; import org.apache.hadoop.hbase.testclassification.ClientTests; import org.apache.hadoop.hbase.testclassification.SmallTests; -import org.junit.Assert; import org.junit.ClassRule; import org.junit.Test; import org.junit.experimental.categories.Category; @@ -49,12 +51,12 @@ public void testUpdateStats() { // Check that the tracker was updated as expected ServerStatistics stats = serverStatisticTracker.getStats(server); - Assert.assertEquals(stats.getStatsForRegion(regionName).getMemStoreLoadPercent(), - regionLoadStats.memstoreLoad); - Assert.assertEquals(stats.getStatsForRegion(regionName).getCompactionPressure(), - regionLoadStats.compactionPressure); - Assert.assertEquals(stats.getStatsForRegion(regionName).getHeapOccupancyPercent(), - regionLoadStats.heapOccupancy); + assertEquals(regionLoadStats.memstoreLoad, stats.getStatsForRegion(regionName) + .getMemStoreLoadPercent()); + assertEquals(regionLoadStats.compactionPressure, stats.getStatsForRegion(regionName) + .getCompactionPressure()); + assertEquals(regionLoadStats.heapOccupancy, stats.getStatsForRegion(regionName) + .getHeapOccupancyPercent()); } @Test @@ -64,7 +66,7 @@ public void testUpdateStatsRegionNameNull() { ResultStatsUtil.updateStats(serverStatisticTracker, server, null, regionLoadStats); ServerStatistics stats = serverStatisticTracker.getStats(server); - Assert.assertEquals(stats,null); + assertNull(stats); } @Test @@ -74,7 +76,7 @@ public void testUpdateStatsStatsNull() { ResultStatsUtil.updateStats(serverStatisticTracker, server, regionName, null); ServerStatistics stats = serverStatisticTracker.getStats(server); - Assert.assertEquals(stats,null); + assertNull(stats); } @Test @@ -84,6 +86,6 @@ public void testUpdateStatsTrackerNull() { ResultStatsUtil.updateStats(null, server, regionName, regionLoadStats); ServerStatistics stats = serverStatisticTracker.getStats(server); - Assert.assertEquals(stats,null); + assertNull(stats); } -} \ No newline at end of file +}