From 94fe90acfeb30f3631a11ebe2ce31feb4a00b165 Mon Sep 17 00:00:00 2001 From: Ray Mattingly Date: Wed, 19 Apr 2023 15:46:40 -0400 Subject: [PATCH 01/11] HBASE-27799: fix misleading RpcThrottlingException message --- .../hadoop/hbase/quotas/RpcThrottlingException.java | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/quotas/RpcThrottlingException.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/quotas/RpcThrottlingException.java index 1cb8abf490d5..66473ee7acf7 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/quotas/RpcThrottlingException.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/quotas/RpcThrottlingException.java @@ -130,10 +130,19 @@ public static void throwWriteCapacityUnitExceeded(final long waitInterval) private static void throwThrottlingException(final Type type, final long waitInterval) throws RpcThrottlingException { - String msg = MSG_TYPE[type.ordinal()] + MSG_WAIT + StringUtils.formatTime(waitInterval); + String msg = MSG_TYPE[type.ordinal()] + MSG_WAIT + stringFromMillis(waitInterval); throw new RpcThrottlingException(type, waitInterval, msg); } + private static String stringFromMillis(long millis) { + if (millis >= 1000) { + return StringUtils.formatTime(millis); + } else { + // StringUtils#formatTime doesn't support millis + return millis + "ms"; + } + } + private static long timeFromString(String timeDiff) { Pattern[] patterns = new Pattern[] { Pattern.compile("^(\\d+\\.\\d\\d)sec"), Pattern.compile("^(\\d+)mins, (\\d+\\.\\d\\d)sec"), From fec0eda92d7e4f040f455a31711c98c704f74f67 Mon Sep 17 00:00:00 2001 From: Ray Mattingly Date: Wed, 19 Apr 2023 19:00:27 -0400 Subject: [PATCH 02/11] add TestRpcThrottlingException --- .../quotas/TestRpcThrottlingException.java | 62 +++++++++++++++++++ 1 file changed, 62 insertions(+) create mode 100644 hbase-client/src/test/java/org/apache/hadoop/hbase/quotas/TestRpcThrottlingException.java diff --git a/hbase-client/src/test/java/org/apache/hadoop/hbase/quotas/TestRpcThrottlingException.java b/hbase-client/src/test/java/org/apache/hadoop/hbase/quotas/TestRpcThrottlingException.java new file mode 100644 index 000000000000..1c89491a4a43 --- /dev/null +++ b/hbase-client/src/test/java/org/apache/hadoop/hbase/quotas/TestRpcThrottlingException.java @@ -0,0 +1,62 @@ +/* + * 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.quotas; + +import static org.junit.Assert.assertTrue; + +import org.apache.hadoop.hbase.HBaseClassTestRule; +import org.apache.hadoop.hbase.testclassification.SmallTests; +import org.junit.ClassRule; +import org.junit.Test; +import org.junit.experimental.categories.Category; + +@Category({ SmallTests.class }) +public class TestRpcThrottlingException { + + @ClassRule + public static final HBaseClassTestRule CLASS_RULE = + HBaseClassTestRule.forClass(TestRpcThrottlingException.class); + + @Test + public void itHandles1sWaitIntervalMessage() { + try { + RpcThrottlingException.throwNumReadRequestsExceeded(1000); + } catch (RpcThrottlingException e) { + assertTrue(e.getMessage().contains("wait 1sec")); + } + } + + @Test + public void itHandles50msWaitIntervalMessage() { + try { + RpcThrottlingException.throwNumReadRequestsExceeded(50); + } catch (RpcThrottlingException e) { + assertTrue(e.getMessage().contains("wait 50ms")); + } + } + + @Test + public void itHandles1min1sWaitIntervalMessage() { + try { + RpcThrottlingException.throwNumReadRequestsExceeded(61_000); + } catch (RpcThrottlingException e) { + assertTrue(e.getMessage().contains("wait 1mins, 1sec")); + } + } + +} From ca2332253d6ec0da34599c67a72509ac05b9343a Mon Sep 17 00:00:00 2001 From: Ray Mattingly Date: Thu, 20 Apr 2023 08:38:37 -0400 Subject: [PATCH 03/11] include ms on all RpcThrottlingException messages --- .../hbase/quotas/RpcThrottlingException.java | 28 +++++++++++++++---- .../quotas/TestRpcThrottlingException.java | 14 +++++----- 2 files changed, 29 insertions(+), 13 deletions(-) diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/quotas/RpcThrottlingException.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/quotas/RpcThrottlingException.java index 66473ee7acf7..3f2a0363d344 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/quotas/RpcThrottlingException.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/quotas/RpcThrottlingException.java @@ -20,7 +20,6 @@ import java.util.regex.Matcher; import java.util.regex.Pattern; import org.apache.hadoop.hbase.HBaseIOException; -import org.apache.hadoop.util.StringUtils; import org.apache.yetus.audience.InterfaceAudience; /** @@ -135,12 +134,29 @@ private static void throwThrottlingException(final Type type, final long waitInt } private static String stringFromMillis(long millis) { - if (millis >= 1000) { - return StringUtils.formatTime(millis); - } else { - // StringUtils#formatTime doesn't support millis - return millis + "ms"; + StringBuilder buf = new StringBuilder(); + long hours = millis / (60 * 60 * 1000); + long rem = (millis % (60 * 60 * 1000)); + long minutes = rem / (60 * 1000); + rem = rem % (60 * 1000); + long seconds = rem / 1000; + long milliseconds = rem % 1000; + + if (hours != 0) { + buf.append(hours); + buf.append("hrs, "); } + if (minutes != 0) { + buf.append(minutes); + buf.append("mins, "); + } + if (seconds != 0) { + buf.append(seconds); + buf.append("sec, "); + } + buf.append(milliseconds); + buf.append("ms"); + return buf.toString(); } private static long timeFromString(String timeDiff) { diff --git a/hbase-client/src/test/java/org/apache/hadoop/hbase/quotas/TestRpcThrottlingException.java b/hbase-client/src/test/java/org/apache/hadoop/hbase/quotas/TestRpcThrottlingException.java index 1c89491a4a43..c2f8a8a14172 100644 --- a/hbase-client/src/test/java/org/apache/hadoop/hbase/quotas/TestRpcThrottlingException.java +++ b/hbase-client/src/test/java/org/apache/hadoop/hbase/quotas/TestRpcThrottlingException.java @@ -33,16 +33,16 @@ public class TestRpcThrottlingException { HBaseClassTestRule.forClass(TestRpcThrottlingException.class); @Test - public void itHandles1sWaitIntervalMessage() { + public void itHandlesSecWaitIntervalMessage() { try { - RpcThrottlingException.throwNumReadRequestsExceeded(1000); + RpcThrottlingException.throwNumReadRequestsExceeded(1001); } catch (RpcThrottlingException e) { - assertTrue(e.getMessage().contains("wait 1sec")); + assertTrue(e.getMessage().contains("wait 1sec, 1ms")); } } @Test - public void itHandles50msWaitIntervalMessage() { + public void itHandlesMsWaitIntervalMessage() { try { RpcThrottlingException.throwNumReadRequestsExceeded(50); } catch (RpcThrottlingException e) { @@ -51,11 +51,11 @@ public void itHandles50msWaitIntervalMessage() { } @Test - public void itHandles1min1sWaitIntervalMessage() { + public void itHandlesMinWaitIntervalMessage() { try { - RpcThrottlingException.throwNumReadRequestsExceeded(61_000); + RpcThrottlingException.throwNumReadRequestsExceeded(65_015); } catch (RpcThrottlingException e) { - assertTrue(e.getMessage().contains("wait 1mins, 1sec")); + assertTrue(e.getMessage().contains("wait 1mins, 5sec, 15ms")); } } From e87b3ed72e32e8988366ee23007b98e500ce9706 Mon Sep 17 00:00:00 2001 From: Ray Mattingly Date: Fri, 21 Apr 2023 11:31:49 -0400 Subject: [PATCH 04/11] javac: include fail to enforce exception expectations --- .../hadoop/hbase/quotas/TestRpcThrottlingException.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/hbase-client/src/test/java/org/apache/hadoop/hbase/quotas/TestRpcThrottlingException.java b/hbase-client/src/test/java/org/apache/hadoop/hbase/quotas/TestRpcThrottlingException.java index c2f8a8a14172..32f8c2f175c4 100644 --- a/hbase-client/src/test/java/org/apache/hadoop/hbase/quotas/TestRpcThrottlingException.java +++ b/hbase-client/src/test/java/org/apache/hadoop/hbase/quotas/TestRpcThrottlingException.java @@ -18,6 +18,7 @@ package org.apache.hadoop.hbase.quotas; import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; import org.apache.hadoop.hbase.HBaseClassTestRule; import org.apache.hadoop.hbase.testclassification.SmallTests; @@ -36,6 +37,7 @@ public class TestRpcThrottlingException { public void itHandlesSecWaitIntervalMessage() { try { RpcThrottlingException.throwNumReadRequestsExceeded(1001); + fail(); } catch (RpcThrottlingException e) { assertTrue(e.getMessage().contains("wait 1sec, 1ms")); } @@ -45,6 +47,7 @@ public void itHandlesSecWaitIntervalMessage() { public void itHandlesMsWaitIntervalMessage() { try { RpcThrottlingException.throwNumReadRequestsExceeded(50); + fail(); } catch (RpcThrottlingException e) { assertTrue(e.getMessage().contains("wait 50ms")); } @@ -54,6 +57,7 @@ public void itHandlesMsWaitIntervalMessage() { public void itHandlesMinWaitIntervalMessage() { try { RpcThrottlingException.throwNumReadRequestsExceeded(65_015); + fail(); } catch (RpcThrottlingException e) { assertTrue(e.getMessage().contains("wait 1mins, 5sec, 15ms")); } From 2c3ac825755687ccb6c1c31510d8eb5480a79ff1 Mon Sep 17 00:00:00 2001 From: Ray Mattingly Date: Wed, 26 Apr 2023 14:44:30 -0400 Subject: [PATCH 05/11] fix timeFromString --- .../hbase/quotas/RpcThrottlingException.java | 34 +++++++++++++------ .../quotas/TestRpcThrottlingException.java | 13 +++++++ 2 files changed, 37 insertions(+), 10 deletions(-) diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/quotas/RpcThrottlingException.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/quotas/RpcThrottlingException.java index 3f2a0363d344..0a2adddaba21 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/quotas/RpcThrottlingException.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/quotas/RpcThrottlingException.java @@ -133,7 +133,8 @@ private static void throwThrottlingException(final Type type, final long waitInt throw new RpcThrottlingException(type, waitInterval, msg); } - private static String stringFromMillis(long millis) { + // Visible for TestRpcThrottlingException + protected static String stringFromMillis(long millis) { StringBuilder buf = new StringBuilder(); long hours = millis / (60 * 60 * 1000); long rem = (millis % (60 * 60 * 1000)); @@ -159,20 +160,33 @@ private static String stringFromMillis(long millis) { return buf.toString(); } - private static long timeFromString(String timeDiff) { - Pattern[] patterns = new Pattern[] { Pattern.compile("^(\\d+\\.\\d\\d)sec"), - Pattern.compile("^(\\d+)mins, (\\d+\\.\\d\\d)sec"), - Pattern.compile("^(\\d+)hrs, (\\d+)mins, (\\d+\\.\\d\\d)sec") }; + // Visible for TestRpcThrottlingException + protected static long timeFromString(String timeDiff) { + Pattern[] patterns = new Pattern[] { Pattern.compile("^(\\d+)ms"), + Pattern.compile("^(\\d+)sec, (\\d+)ms"), Pattern.compile("^(\\d+)mins, (\\d+)sec, (\\d+)ms"), + Pattern.compile("^(\\d+)hrs, (\\d+)mins, (\\d+)sec, (\\d+)ms"), }; for (int i = 0; i < patterns.length; ++i) { Matcher m = patterns[i].matcher(timeDiff); if (m.find()) { - long time = Math.round(Float.parseFloat(m.group(1 + i)) * 1000); - if (i > 0) { - time += Long.parseLong(m.group(i)) * (60 * 1000); + if (i == 0) { + return Math.round(Float.parseFloat(m.group(1))); // ms } - if (i > 1) { - time += Long.parseLong(m.group(i - 1)) * (60 * 60 * 1000); + long time = 0; + if (i == 1) { + time += Math.round(Float.parseFloat(m.group(1)) * 1000); // sec + time += Math.round(Float.parseFloat(m.group(2))); // ms + } + if (i == 2) { + time += Math.round(Float.parseFloat(m.group(1)) * 60 * 1000); // mins + time += Math.round(Float.parseFloat(m.group(2)) * 1000); // sec + time += Math.round(Float.parseFloat(m.group(3))); // ms + } + if (i == 3) { + time += Math.round(Float.parseFloat(m.group(1)) * 60 * 60 * 1000); // hrs + time += Math.round(Float.parseFloat(m.group(2)) * 60 * 1000); // mins + time += Math.round(Float.parseFloat(m.group(3)) * 1000); // sec + time += Math.round(Float.parseFloat(m.group(4))); // ms } return time; } diff --git a/hbase-client/src/test/java/org/apache/hadoop/hbase/quotas/TestRpcThrottlingException.java b/hbase-client/src/test/java/org/apache/hadoop/hbase/quotas/TestRpcThrottlingException.java index 32f8c2f175c4..383cacf5b678 100644 --- a/hbase-client/src/test/java/org/apache/hadoop/hbase/quotas/TestRpcThrottlingException.java +++ b/hbase-client/src/test/java/org/apache/hadoop/hbase/quotas/TestRpcThrottlingException.java @@ -17,6 +17,7 @@ */ package org.apache.hadoop.hbase.quotas; +import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; @@ -63,4 +64,16 @@ public void itHandlesMinWaitIntervalMessage() { } } + @Test + public void itConvertsMillisToString() { + String output = RpcThrottlingException.stringFromMillis(6500); + assertEquals("6sec, 500ms", output); + } + + @Test + public void itConvertsStringToMillis() { + long millis = RpcThrottlingException.timeFromString("5mins, 2sec, 0ms"); + assertEquals(millis, 302000); + } + } From 8de4a19c9a62818bad6d01644ec2dcd074b17461 Mon Sep 17 00:00:00 2001 From: Ray Mattingly Date: Wed, 26 Apr 2023 15:52:24 -0400 Subject: [PATCH 06/11] support legacy timeFromString --- .../hbase/quotas/RpcThrottlingException.java | 32 +++++++++++++++++++ .../quotas/TestRpcThrottlingException.java | 12 +++++++ 2 files changed, 44 insertions(+) diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/quotas/RpcThrottlingException.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/quotas/RpcThrottlingException.java index 0a2adddaba21..475581641a05 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/quotas/RpcThrottlingException.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/quotas/RpcThrottlingException.java @@ -162,6 +162,10 @@ protected static String stringFromMillis(long millis) { // Visible for TestRpcThrottlingException protected static long timeFromString(String timeDiff) { + if (!timeDiff.contains("ms")) { + // legacy timeDiff Strings do not contain ms + return legacyTimeFromString(timeDiff); + } Pattern[] patterns = new Pattern[] { Pattern.compile("^(\\d+)ms"), Pattern.compile("^(\\d+)sec, (\\d+)ms"), Pattern.compile("^(\\d+)mins, (\\d+)sec, (\\d+)ms"), Pattern.compile("^(\\d+)hrs, (\\d+)mins, (\\d+)sec, (\\d+)ms"), }; @@ -194,4 +198,32 @@ protected static long timeFromString(String timeDiff) { return -1; } + + private static long legacyTimeFromString(String timeDiff) { + Pattern[] patterns = + new Pattern[] { Pattern.compile("^(\\d+)sec"), Pattern.compile("^(\\d+)mins, (\\d+)sec"), + Pattern.compile("^(\\d+)hrs, (\\d+)mins, (\\d+)sec"), }; + + for (int i = 0; i < patterns.length; ++i) { + Matcher m = patterns[i].matcher(timeDiff); + if (m.find()) { + if (i == 0) { + return Math.round(Float.parseFloat(m.group(1)) * 1000); // sec + } + long time = 0; + if (i == 1) { + time += Math.round(Float.parseFloat(m.group(1)) * 60 * 1000); // mins + time += Math.round(Float.parseFloat(m.group(2)) * 1000); // sec + } + if (i == 2) { + time += Math.round(Float.parseFloat(m.group(1)) * 60 * 60 * 1000); // hrs + time += Math.round(Float.parseFloat(m.group(2)) * 60 * 1000); // mins + time += Math.round(Float.parseFloat(m.group(3)) * 1000); // sec + } + return time; + } + } + + return -1; + } } diff --git a/hbase-client/src/test/java/org/apache/hadoop/hbase/quotas/TestRpcThrottlingException.java b/hbase-client/src/test/java/org/apache/hadoop/hbase/quotas/TestRpcThrottlingException.java index 383cacf5b678..165e55e067f9 100644 --- a/hbase-client/src/test/java/org/apache/hadoop/hbase/quotas/TestRpcThrottlingException.java +++ b/hbase-client/src/test/java/org/apache/hadoop/hbase/quotas/TestRpcThrottlingException.java @@ -76,4 +76,16 @@ public void itConvertsStringToMillis() { assertEquals(millis, 302000); } + @Test + public void itConvertsLegacyStringSecToMillis() { + long millis = RpcThrottlingException.timeFromString("5sec"); + assertEquals(millis, 5000); + } + + @Test + public void itConvertsLegacyStringHourMinSecToMillis2() { + long millis = RpcThrottlingException.timeFromString("1hrs, 3mins, 5sec"); + assertEquals(millis, 3785000); + } + } From ded03ee4934ec5fda89916968b84cc9e4b0f96da Mon Sep 17 00:00:00 2001 From: Ray Mattingly Date: Wed, 26 Apr 2023 17:01:40 -0400 Subject: [PATCH 07/11] unify timeFromString and DRY --- .../hbase/quotas/RpcThrottlingException.java | 65 ++++--------------- 1 file changed, 14 insertions(+), 51 deletions(-) diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/quotas/RpcThrottlingException.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/quotas/RpcThrottlingException.java index 475581641a05..d68bf5c17db2 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/quotas/RpcThrottlingException.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/quotas/RpcThrottlingException.java @@ -162,68 +162,31 @@ protected static String stringFromMillis(long millis) { // Visible for TestRpcThrottlingException protected static long timeFromString(String timeDiff) { - if (!timeDiff.contains("ms")) { - // legacy timeDiff Strings do not contain ms - return legacyTimeFromString(timeDiff); - } Pattern[] patterns = new Pattern[] { Pattern.compile("^(\\d+)ms"), Pattern.compile("^(\\d+)sec, (\\d+)ms"), Pattern.compile("^(\\d+)mins, (\\d+)sec, (\\d+)ms"), - Pattern.compile("^(\\d+)hrs, (\\d+)mins, (\\d+)sec, (\\d+)ms"), }; - + Pattern.compile("^(\\d+)hrs, (\\d+)mins, (\\d+)sec, (\\d+)ms"), + // below are patterns to support the legacy format which did not include ms + // see HBASE-27799 + Pattern.compile("^(\\d+)sec"), Pattern.compile("^(\\d+)mins, (\\d+)sec"), + Pattern.compile("^(\\d+)hrs, (\\d+)mins, (\\d+)sec"), }; + + long[] factors = new long[] { 60 * 60 * 1000, 60 * 1000, 1000, 1 }; + int factorIndexOffset = 0; for (int i = 0; i < patterns.length; ++i) { - Matcher m = patterns[i].matcher(timeDiff); - if (m.find()) { - if (i == 0) { - return Math.round(Float.parseFloat(m.group(1))); // ms - } - long time = 0; - if (i == 1) { - time += Math.round(Float.parseFloat(m.group(1)) * 1000); // sec - time += Math.round(Float.parseFloat(m.group(2))); // ms - } - if (i == 2) { - time += Math.round(Float.parseFloat(m.group(1)) * 60 * 1000); // mins - time += Math.round(Float.parseFloat(m.group(2)) * 1000); // sec - time += Math.round(Float.parseFloat(m.group(3))); // ms - } - if (i == 3) { - time += Math.round(Float.parseFloat(m.group(1)) * 60 * 60 * 1000); // hrs - time += Math.round(Float.parseFloat(m.group(2)) * 60 * 1000); // mins - time += Math.round(Float.parseFloat(m.group(3)) * 1000); // sec - time += Math.round(Float.parseFloat(m.group(4))); // ms - } - return time; + if (i == 4) { + factorIndexOffset = 3; } - } - - return -1; - } - - private static long legacyTimeFromString(String timeDiff) { - Pattern[] patterns = - new Pattern[] { Pattern.compile("^(\\d+)sec"), Pattern.compile("^(\\d+)mins, (\\d+)sec"), - Pattern.compile("^(\\d+)hrs, (\\d+)mins, (\\d+)sec"), }; - - for (int i = 0; i < patterns.length; ++i) { Matcher m = patterns[i].matcher(timeDiff); if (m.find()) { - if (i == 0) { - return Math.round(Float.parseFloat(m.group(1)) * 1000); // sec - } + int numGroups = m.groupCount(); long time = 0; - if (i == 1) { - time += Math.round(Float.parseFloat(m.group(1)) * 60 * 1000); // mins - time += Math.round(Float.parseFloat(m.group(2)) * 1000); // sec - } - if (i == 2) { - time += Math.round(Float.parseFloat(m.group(1)) * 60 * 60 * 1000); // hrs - time += Math.round(Float.parseFloat(m.group(2)) * 60 * 1000); // mins - time += Math.round(Float.parseFloat(m.group(3)) * 1000); // sec + int startingFactorIndex = factors.length - i + factorIndexOffset; + for (int j = 1; j <= numGroups; j++) { + time += Math.round(Float.parseFloat(m.group(j)) * factors[startingFactorIndex + j - 2]); } return time; } } - return -1; } } From b25ded10241f719def5f7aa8698200c8c9999f24 Mon Sep 17 00:00:00 2001 From: Ray Mattingly Date: Wed, 26 Apr 2023 18:00:07 -0400 Subject: [PATCH 08/11] match 1 pattern --- .../hbase/quotas/RpcThrottlingException.java | 38 +++++++++---------- 1 file changed, 17 insertions(+), 21 deletions(-) diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/quotas/RpcThrottlingException.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/quotas/RpcThrottlingException.java index d68bf5c17db2..eeda15a6129d 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/quotas/RpcThrottlingException.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/quotas/RpcThrottlingException.java @@ -162,30 +162,26 @@ protected static String stringFromMillis(long millis) { // Visible for TestRpcThrottlingException protected static long timeFromString(String timeDiff) { - Pattern[] patterns = new Pattern[] { Pattern.compile("^(\\d+)ms"), - Pattern.compile("^(\\d+)sec, (\\d+)ms"), Pattern.compile("^(\\d+)mins, (\\d+)sec, (\\d+)ms"), - Pattern.compile("^(\\d+)hrs, (\\d+)mins, (\\d+)sec, (\\d+)ms"), - // below are patterns to support the legacy format which did not include ms - // see HBASE-27799 - Pattern.compile("^(\\d+)sec"), Pattern.compile("^(\\d+)mins, (\\d+)sec"), - Pattern.compile("^(\\d+)hrs, (\\d+)mins, (\\d+)sec"), }; - + Pattern pattern; + if (timeDiff.contains("ms")) { + pattern = Pattern.compile("^(?:(\\d+)hrs, )?(?:(\\d+)mins, )?(?:(\\d+)sec, )?(?:(\\d+)ms)?"); + } else { + // legacy pattern. see HBASE-27799 which added millis to this String + pattern = Pattern.compile("^(?:(\\d+)hrs, )?(?:(\\d+)mins, )?(?:(\\d+)sec)?"); + } long[] factors = new long[] { 60 * 60 * 1000, 60 * 1000, 1000, 1 }; - int factorIndexOffset = 0; - for (int i = 0; i < patterns.length; ++i) { - if (i == 4) { - factorIndexOffset = 3; - } - Matcher m = patterns[i].matcher(timeDiff); - if (m.find()) { - int numGroups = m.groupCount(); - long time = 0; - int startingFactorIndex = factors.length - i + factorIndexOffset; - for (int j = 1; j <= numGroups; j++) { - time += Math.round(Float.parseFloat(m.group(j)) * factors[startingFactorIndex + j - 2]); + Matcher m = pattern.matcher(timeDiff); + if (m.find()) { + int numGroups = m.groupCount(); + long time = 0; + for (int j = 1; j <= numGroups; j++) { + String group = m.group(j); + if (group == null) { + continue; } - return time; + time += Math.round(Float.parseFloat(group) * factors[j - 1]); } + return time; } return -1; } From dfa5f5e6de540f8aadac47c42ca3ecd82b66fd07 Mon Sep 17 00:00:00 2001 From: Ray Mattingly Date: Wed, 26 Apr 2023 18:28:46 -0400 Subject: [PATCH 09/11] only 1 pattern --- .../hadoop/hbase/quotas/RpcThrottlingException.java | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/quotas/RpcThrottlingException.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/quotas/RpcThrottlingException.java index eeda15a6129d..30a9f7dfaf29 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/quotas/RpcThrottlingException.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/quotas/RpcThrottlingException.java @@ -162,13 +162,8 @@ protected static String stringFromMillis(long millis) { // Visible for TestRpcThrottlingException protected static long timeFromString(String timeDiff) { - Pattern pattern; - if (timeDiff.contains("ms")) { - pattern = Pattern.compile("^(?:(\\d+)hrs, )?(?:(\\d+)mins, )?(?:(\\d+)sec, )?(?:(\\d+)ms)?"); - } else { - // legacy pattern. see HBASE-27799 which added millis to this String - pattern = Pattern.compile("^(?:(\\d+)hrs, )?(?:(\\d+)mins, )?(?:(\\d+)sec)?"); - } + Pattern pattern = + Pattern.compile("^(?:(\\d+)hrs, )?(?:(\\d+)mins, )?(?:(\\d+)sec.{0,2})?(?:(\\d+)ms)?"); long[] factors = new long[] { 60 * 60 * 1000, 60 * 1000, 1000, 1 }; Matcher m = pattern.matcher(timeDiff); if (m.find()) { From c5d1e49b07216f8db1a90e785a6e65fa31f91955 Mon Sep 17 00:00:00 2001 From: Ray Mattingly Date: Tue, 2 May 2023 08:34:36 -0400 Subject: [PATCH 10/11] pr feedback: better tests, cleaner copy --- .../hbase/quotas/RpcThrottlingException.java | 6 +- .../quotas/TestRpcThrottlingException.java | 68 +++++++------------ 2 files changed, 27 insertions(+), 47 deletions(-) diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/quotas/RpcThrottlingException.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/quotas/RpcThrottlingException.java index 30a9f7dfaf29..2c1f13e94e66 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/quotas/RpcThrottlingException.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/quotas/RpcThrottlingException.java @@ -145,11 +145,11 @@ protected static String stringFromMillis(long millis) { if (hours != 0) { buf.append(hours); - buf.append("hrs, "); + buf.append(hours > 1 ? "hrs, " : "hr, "); } if (minutes != 0) { buf.append(minutes); - buf.append("mins, "); + buf.append(minutes > 1 ? "mins, " : "min, "); } if (seconds != 0) { buf.append(seconds); @@ -163,7 +163,7 @@ protected static String stringFromMillis(long millis) { // Visible for TestRpcThrottlingException protected static long timeFromString(String timeDiff) { Pattern pattern = - Pattern.compile("^(?:(\\d+)hrs, )?(?:(\\d+)mins, )?(?:(\\d+)sec.{0,2})?(?:(\\d+)ms)?"); + Pattern.compile("^(?:(\\d+)hrs?, )?(?:(\\d+)mins?, )?(?:(\\d+)sec[, ]{0,2})?(?:(\\d+)ms)?"); long[] factors = new long[] { 60 * 60 * 1000, 60 * 1000, 1000, 1 }; Matcher m = pattern.matcher(timeDiff); if (m.find()) { diff --git a/hbase-client/src/test/java/org/apache/hadoop/hbase/quotas/TestRpcThrottlingException.java b/hbase-client/src/test/java/org/apache/hadoop/hbase/quotas/TestRpcThrottlingException.java index 165e55e067f9..b5262f12b054 100644 --- a/hbase-client/src/test/java/org/apache/hadoop/hbase/quotas/TestRpcThrottlingException.java +++ b/hbase-client/src/test/java/org/apache/hadoop/hbase/quotas/TestRpcThrottlingException.java @@ -18,15 +18,16 @@ package org.apache.hadoop.hbase.quotas; import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertTrue; -import static org.junit.Assert.fail; +import java.util.Map; import org.apache.hadoop.hbase.HBaseClassTestRule; import org.apache.hadoop.hbase.testclassification.SmallTests; import org.junit.ClassRule; import org.junit.Test; import org.junit.experimental.categories.Category; +import org.apache.hbase.thirdparty.com.google.common.collect.ImmutableMap; + @Category({ SmallTests.class }) public class TestRpcThrottlingException { @@ -34,58 +35,37 @@ public class TestRpcThrottlingException { public static final HBaseClassTestRule CLASS_RULE = HBaseClassTestRule.forClass(TestRpcThrottlingException.class); - @Test - public void itHandlesSecWaitIntervalMessage() { - try { - RpcThrottlingException.throwNumReadRequestsExceeded(1001); - fail(); - } catch (RpcThrottlingException e) { - assertTrue(e.getMessage().contains("wait 1sec, 1ms")); - } - } + private static final Map STR_TO_MS_NEW_FORMAT = ImmutableMap. builder().put("0ms", 0L) + .put("50ms", 50L).put("1sec, 1ms", 1001L).put("1min, 5sec, 15ms", 65_015L) + .put("5mins, 2sec, 0ms", 302000L).put("1hr, 3mins, 5sec, 1ms", 3785001L).build(); + private static final Map STR_TO_MS_LEGACY_FORMAT = ImmutableMap. builder().put("0sec", 0L) + .put("1sec", 1000L).put("2sec", 2000L).put("1mins, 5sec", 65_000L).put("5mins, 2sec", 302000L) + .put("1hrs, 3mins, 5sec", 3785000L).build(); @Test - public void itHandlesMsWaitIntervalMessage() { - try { - RpcThrottlingException.throwNumReadRequestsExceeded(50); - fail(); - } catch (RpcThrottlingException e) { - assertTrue(e.getMessage().contains("wait 50ms")); + public void itConvertsMillisToNewString() { + for (Map.Entry strAndMs : STR_TO_MS_NEW_FORMAT.entrySet()) { + String output = RpcThrottlingException.stringFromMillis(strAndMs.getValue()); + assertEquals(strAndMs.getKey(), output); } } @Test - public void itHandlesMinWaitIntervalMessage() { - try { - RpcThrottlingException.throwNumReadRequestsExceeded(65_015); - fail(); - } catch (RpcThrottlingException e) { - assertTrue(e.getMessage().contains("wait 1mins, 5sec, 15ms")); + public void itConvertsNewStringToMillis() { + for (Map.Entry strAndMs : STR_TO_MS_NEW_FORMAT.entrySet()) { + Long output = RpcThrottlingException.timeFromString(strAndMs.getKey()); + assertEquals(strAndMs.getValue(), output); } } @Test - public void itConvertsMillisToString() { - String output = RpcThrottlingException.stringFromMillis(6500); - assertEquals("6sec, 500ms", output); - } - - @Test - public void itConvertsStringToMillis() { - long millis = RpcThrottlingException.timeFromString("5mins, 2sec, 0ms"); - assertEquals(millis, 302000); - } - - @Test - public void itConvertsLegacyStringSecToMillis() { - long millis = RpcThrottlingException.timeFromString("5sec"); - assertEquals(millis, 5000); - } - - @Test - public void itConvertsLegacyStringHourMinSecToMillis2() { - long millis = RpcThrottlingException.timeFromString("1hrs, 3mins, 5sec"); - assertEquals(millis, 3785000); + public void itConvertsLegacyStringToMillis() { + for (Map.Entry strAndMs : STR_TO_MS_LEGACY_FORMAT.entrySet()) { + Long output = RpcThrottlingException.timeFromString(strAndMs.getKey()); + assertEquals(strAndMs.getValue(), output); + } } } From cdeb4a76408623e48bf949c6c2ddb86e2875bd2e Mon Sep 17 00:00:00 2001 From: Ray Mattingly Date: Wed, 3 May 2023 17:58:39 -0400 Subject: [PATCH 11/11] more test cases --- .../hadoop/hbase/quotas/TestRpcThrottlingException.java | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/hbase-client/src/test/java/org/apache/hadoop/hbase/quotas/TestRpcThrottlingException.java b/hbase-client/src/test/java/org/apache/hadoop/hbase/quotas/TestRpcThrottlingException.java index b5262f12b054..21b01ad4764a 100644 --- a/hbase-client/src/test/java/org/apache/hadoop/hbase/quotas/TestRpcThrottlingException.java +++ b/hbase-client/src/test/java/org/apache/hadoop/hbase/quotas/TestRpcThrottlingException.java @@ -35,10 +35,11 @@ public class TestRpcThrottlingException { public static final HBaseClassTestRule CLASS_RULE = HBaseClassTestRule.forClass(TestRpcThrottlingException.class); - private static final Map STR_TO_MS_NEW_FORMAT = ImmutableMap. builder().put("0ms", 0L) - .put("50ms", 50L).put("1sec, 1ms", 1001L).put("1min, 5sec, 15ms", 65_015L) - .put("5mins, 2sec, 0ms", 302000L).put("1hr, 3mins, 5sec, 1ms", 3785001L).build(); + private static final Map STR_TO_MS_NEW_FORMAT = + ImmutableMap. builder().put("0ms", 0L).put("50ms", 50L).put("1sec, 1ms", 1001L) + .put("1min, 5sec, 15ms", 65_015L).put("5mins, 2sec, 0ms", 302000L) + .put("1hr, 3mins, 5sec, 1ms", 3785001L).put("1hr, 5sec, 1ms", 3605001L) + .put("1hr, 0ms", 3600000L).put("1hr, 1min, 1ms", 3660001L).build(); private static final Map STR_TO_MS_LEGACY_FORMAT = ImmutableMap. builder().put("0sec", 0L) .put("1sec", 1000L).put("2sec", 2000L).put("1mins, 5sec", 65_000L).put("5mins, 2sec", 302000L)