From 6479159db435e7a4dc1838a37c749984d7a5823a Mon Sep 17 00:00:00 2001 From: Xing Lin Date: Tue, 21 Nov 2023 15:28:07 -0800 Subject: [PATCH 1/3] Fixed the verbose log.warn in DFSUtil.addTransferRateMetric(). Contributed by Ravindra Dingankar . --- .../java/org/apache/hadoop/hdfs/DFSUtil.java | 29 ++++++++++++++----- .../hdfs/server/datanode/DataXceiver.java | 16 +++++----- .../org/apache/hadoop/hdfs/TestDFSUtil.java | 17 +++++++---- 3 files changed, 41 insertions(+), 21 deletions(-) diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSUtil.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSUtil.java index 1b3e77e0fe4656..b9740351dfb9f6 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSUtil.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSUtil.java @@ -60,6 +60,7 @@ import java.util.Set; import java.util.SortedSet; +import java.util.concurrent.TimeUnit; import org.apache.commons.cli.CommandLine; import org.apache.commons.cli.CommandLineParser; import org.apache.commons.cli.Option; @@ -1970,16 +1971,28 @@ public static boolean isParentEntry(final String path, final String parent) { } /** - * Add transfer rate metrics for valid data read and duration values. + * Add transfer rate metrics in bytes per second. * @param metrics metrics for datanodes * @param read bytes read - * @param duration read duration + * @param durationInNS read duration in nanoseconds */ - public static void addTransferRateMetric(final DataNodeMetrics metrics, final long read, final long duration) { - if (read >= 0 && duration > 0) { - metrics.addReadTransferRate(read * 1000 / duration); - } else { - LOG.warn("Unexpected value for data transfer bytes={} duration={}", read, duration); - } + public static void addTransferRateMetric(final DataNodeMetrics metrics, final long read, final long durationInNS) { + metrics.addReadTransferRate(getTransferRateInBytesPerSecond(read, durationInNS)); + } + + /** + * We have the read duration in nanoseconds for precision for transfers taking a few nanoseconds. + * We treat shorter durations below 1 ns as 1 ns as we also want to capture reads taking less than a nanosecond. + * To calculate transferRate in bytes per second, we avoid multiplying bytes read by 10^9 to avoid overflow. + * Instead we first calculate the duration in seconds in double to keep the decimal values for smaller durations. + * We then divide bytes read by durationInSeconds to get the transferRate in bytes per second. + * @param bytes + * @param durationInNS + * @return + */ + public static long getTransferRateInBytesPerSecond(final long bytes, long durationInNS) { + durationInNS = Math.max(durationInNS, 1); + double durationInSeconds = (double) durationInNS / TimeUnit.SECONDS.toNanos(1); + return (long) (bytes / durationInSeconds); } } diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/DataXceiver.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/DataXceiver.java index e97e179702970d..017e57012be8bf 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/DataXceiver.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/DataXceiver.java @@ -607,10 +607,10 @@ public void readBlock(final ExtendedBlock block, // send op status writeSuccessWithChecksumInfo(blockSender, new DataOutputStream(getOutputStream())); - long beginRead = Time.monotonicNow(); + long beginReadInNS = Time.monotonicNowNanos(); // send data read = blockSender.sendBlock(out, baseStream, dataXceiverServer.getReadThrottler()); - long duration = Time.monotonicNow() - beginRead; + long durationInNS = Time.monotonicNowNanos() - beginReadInNS; if (blockSender.didSendEntireByteRange()) { // If we sent the entire range, then we should expect the client // to respond with a Status enum. @@ -633,8 +633,8 @@ public void readBlock(final ExtendedBlock block, } datanode.metrics.incrBytesRead((int) read); datanode.metrics.incrBlocksRead(); - datanode.metrics.incrTotalReadTime(duration); - DFSUtil.addTransferRateMetric(datanode.metrics, read, duration); + datanode.metrics.incrTotalReadTime(TimeUnit.NANOSECONDS.toMillis(durationInNS)); + DFSUtil.addTransferRateMetric(datanode.metrics, read, durationInNS); } catch ( SocketException ignored ) { LOG.trace("{}:Ignoring exception while serving {} to {}", dnR, block, remoteAddress, ignored); @@ -1117,15 +1117,15 @@ public void copyBlock(final ExtendedBlock block, // send status first writeSuccessWithChecksumInfo(blockSender, reply); - long beginRead = Time.monotonicNow(); + long beginReadInNS = Time.monotonicNowNanos(); // send block content to the target long read = blockSender.sendBlock(reply, baseStream, dataXceiverServer.balanceThrottler); - long duration = Time.monotonicNow() - beginRead; + long durationInNS = Time.monotonicNowNanos() - beginReadInNS; datanode.metrics.incrBytesRead((int) read); datanode.metrics.incrBlocksRead(); - datanode.metrics.incrTotalReadTime(duration); - DFSUtil.addTransferRateMetric(datanode.metrics, read, duration); + datanode.metrics.incrTotalReadTime(TimeUnit.NANOSECONDS.toMillis(durationInNS)); + DFSUtil.addTransferRateMetric(datanode.metrics, read, durationInNS); LOG.info("Copied {} to {}", block, peer.getRemoteAddressString()); } catch (IOException ioe) { diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSUtil.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSUtil.java index 5d7110d3d9a8bc..5305056aad266c 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSUtil.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSUtil.java @@ -1127,14 +1127,21 @@ public void testErrorMessageForInvalidNameservice() throws Exception { @Test public void testAddTransferRateMetricForValidValues() { DataNodeMetrics mockMetrics = mock(DataNodeMetrics.class); - DFSUtil.addTransferRateMetric(mockMetrics, 100, 10); - verify(mockMetrics).addReadTransferRate(10000); + DFSUtil.addTransferRateMetric(mockMetrics, 3_251_854_872L, 129_593_000_000L); + verify(mockMetrics).addReadTransferRate(250_92_828L); } @Test - public void testAddTransferRateMetricForInvalidValue() { + public void testAddTransferRateMetricFor0Nanosecond() { DataNodeMetrics mockMetrics = mock(DataNodeMetrics.class); - DFSUtil.addTransferRateMetric(mockMetrics, 100, 0); - verify(mockMetrics, times(0)).addReadTransferRate(anyLong()); + DFSUtil.addTransferRateMetric(mockMetrics, 1L, 0); + verify(mockMetrics).addReadTransferRate(999_999_999L); + } + + @Test + public void testGetTransferRateInBytesPerSecond() { + assertEquals(999_999_999, DFSUtil.getTransferRateInBytesPerSecond(1L, 1L)); + assertEquals(999_999_999, DFSUtil.getTransferRateInBytesPerSecond(1L, 0L)); + assertEquals(102_400_000, DFSUtil.getTransferRateInBytesPerSecond(512_000_000L, 5_000_000_000L)); } } From 2dcde2dd7553b27e13d42c3322181b54f3a7f2bf Mon Sep 17 00:00:00 2001 From: Xing Lin Date: Wed, 22 Nov 2023 08:55:56 -0800 Subject: [PATCH 2/3] Fixed checkstyle/javadoc warnings --- .../java/org/apache/hadoop/hdfs/DFSUtil.java | 20 +++++++++++-------- .../org/apache/hadoop/hdfs/TestDFSUtil.java | 7 ++++--- 2 files changed, 16 insertions(+), 11 deletions(-) diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSUtil.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSUtil.java index b9740351dfb9f6..be7364cf276fd2 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSUtil.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSUtil.java @@ -1976,19 +1976,23 @@ public static boolean isParentEntry(final String path, final String parent) { * @param read bytes read * @param durationInNS read duration in nanoseconds */ - public static void addTransferRateMetric(final DataNodeMetrics metrics, final long read, final long durationInNS) { + public static void addTransferRateMetric(final DataNodeMetrics metrics, final long read, + final long durationInNS) { metrics.addReadTransferRate(getTransferRateInBytesPerSecond(read, durationInNS)); } /** + * Calculate the transfer rate in bytes per second. + * * We have the read duration in nanoseconds for precision for transfers taking a few nanoseconds. - * We treat shorter durations below 1 ns as 1 ns as we also want to capture reads taking less than a nanosecond. - * To calculate transferRate in bytes per second, we avoid multiplying bytes read by 10^9 to avoid overflow. - * Instead we first calculate the duration in seconds in double to keep the decimal values for smaller durations. - * We then divide bytes read by durationInSeconds to get the transferRate in bytes per second. - * @param bytes - * @param durationInNS - * @return + * We treat shorter durations below 1 ns as 1 ns as we also want to capture reads taking less + * than a nanosecond. To calculate transferRate in bytes per second, we avoid multiplying bytes + * read by 10^9 to avoid overflow. Instead, we first calculate the duration in seconds in double + * to keep the decimal values for smaller durations. We then divide bytes read by + * durationInSeconds to get the transferRate in bytes per second. + * @param bytes bytes read + * @param durationInNS read duration in nanoseconds + * @return bytes per second */ public static long getTransferRateInBytesPerSecond(final long bytes, long durationInNS) { durationInNS = Math.max(durationInNS, 1); diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSUtil.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSUtil.java index 5305056aad266c..321eb06aade1a3 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSUtil.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSUtil.java @@ -1140,8 +1140,9 @@ public void testAddTransferRateMetricFor0Nanosecond() { @Test public void testGetTransferRateInBytesPerSecond() { - assertEquals(999_999_999, DFSUtil.getTransferRateInBytesPerSecond(1L, 1L)); - assertEquals(999_999_999, DFSUtil.getTransferRateInBytesPerSecond(1L, 0L)); - assertEquals(102_400_000, DFSUtil.getTransferRateInBytesPerSecond(512_000_000L, 5_000_000_000L)); + assertEquals(999_999_999, DFSUtil.getTransferRateInBytesPerSecond(1L, 1L)); + assertEquals(999_999_999, DFSUtil.getTransferRateInBytesPerSecond(1L, 0L)); + assertEquals(102_400_000, + DFSUtil.getTransferRateInBytesPerSecond(512_000_000L, 5_000_000_000L)); } } From c18693adfb9f77959f4732bda9b0b65c2af160b1 Mon Sep 17 00:00:00 2001 From: Xing Lin Date: Mon, 4 Dec 2023 10:16:33 -0800 Subject: [PATCH 3/3] Replace negative transfer bytes with 0 byte. --- .../java/org/apache/hadoop/hdfs/DFSUtil.java | 6 +++++- .../java/org/apache/hadoop/hdfs/TestDFSUtil.java | 16 +++++++++++++++- 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSUtil.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSUtil.java index be7364cf276fd2..3ecb20bc6a2233 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSUtil.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSUtil.java @@ -1990,11 +1990,15 @@ public static void addTransferRateMetric(final DataNodeMetrics metrics, final lo * read by 10^9 to avoid overflow. Instead, we first calculate the duration in seconds in double * to keep the decimal values for smaller durations. We then divide bytes read by * durationInSeconds to get the transferRate in bytes per second. + * + * We also replace a negative value for transferred bytes with 0 byte. + * * @param bytes bytes read * @param durationInNS read duration in nanoseconds * @return bytes per second */ - public static long getTransferRateInBytesPerSecond(final long bytes, long durationInNS) { + public static long getTransferRateInBytesPerSecond(long bytes, long durationInNS) { + bytes = Math.max(bytes, 0); durationInNS = Math.max(durationInNS, 1); double durationInSeconds = (double) durationInNS / TimeUnit.SECONDS.toNanos(1); return (long) (bytes / durationInSeconds); diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSUtil.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSUtil.java index 321eb06aade1a3..4bdb405e4da080 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSUtil.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSUtil.java @@ -1132,12 +1132,26 @@ public void testAddTransferRateMetricForValidValues() { } @Test - public void testAddTransferRateMetricFor0Nanosecond() { + public void testAddTransferRateMetricForZeroNSTransferDuration() { DataNodeMetrics mockMetrics = mock(DataNodeMetrics.class); DFSUtil.addTransferRateMetric(mockMetrics, 1L, 0); verify(mockMetrics).addReadTransferRate(999_999_999L); } + @Test + public void testAddTransferRateMetricNegativeTransferBytes() { + DataNodeMetrics mockMetrics = mock(DataNodeMetrics.class); + DFSUtil.addTransferRateMetric(mockMetrics, -1L, 0); + verify(mockMetrics).addReadTransferRate(0L); + } + + @Test + public void testAddTransferRateMetricZeroTransferBytes() { + DataNodeMetrics mockMetrics = mock(DataNodeMetrics.class); + DFSUtil.addTransferRateMetric(mockMetrics, -1L, 0); + verify(mockMetrics).addReadTransferRate(0L); + } + @Test public void testGetTransferRateInBytesPerSecond() { assertEquals(999_999_999, DFSUtil.getTransferRateInBytesPerSecond(1L, 1L));