kgeisz commented on code in PR #7186: URL: https://github.com/apache/hbase/pull/7186#discussion_r2249388151
########## hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/metrics/TestMetricsThrottleExceptions.java: ########## @@ -0,0 +1,319 @@ +/* + * 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.regionserver.metrics; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +import java.util.Optional; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; +import org.apache.hadoop.hbase.HBaseClassTestRule; +import org.apache.hadoop.hbase.metrics.Counter; +import org.apache.hadoop.hbase.metrics.Metric; +import org.apache.hadoop.hbase.metrics.MetricRegistries; +import org.apache.hadoop.hbase.metrics.MetricRegistry; +import org.apache.hadoop.hbase.metrics.MetricRegistryInfo; +import org.apache.hadoop.hbase.quotas.RpcThrottlingException; +import org.apache.hadoop.hbase.testclassification.RegionServerTests; +import org.apache.hadoop.hbase.testclassification.SmallTests; +import org.junit.After; +import org.junit.ClassRule; +import org.junit.Test; +import org.junit.experimental.categories.Category; + +@Category({ RegionServerTests.class, SmallTests.class }) +public class TestMetricsThrottleExceptions { + + @ClassRule + public static final HBaseClassTestRule CLASS_RULE = + HBaseClassTestRule.forClass(TestMetricsThrottleExceptions.class); + + @After + public void cleanup() { + // Clean up global registries after each test to avoid interference + MetricRegistries.global().clear(); + } + + @Test + public void testBasicThrottleMetricsRecording() { + // Create a test registry for the metrics + MetricRegistryInfo registryInfo = getRegistryInfo(); + MetricRegistry testRegistry = MetricRegistries.global().create(registryInfo); + MetricsThrottleExceptions throttleMetrics = new MetricsThrottleExceptions(testRegistry); + + // Record a throttle exception + throttleMetrics.recordThrottleException(RpcThrottlingException.Type.NumRequestsExceeded, + "alice", "users"); + + // Verify the counter exists and has correct value + Optional<Metric> metric = + testRegistry.get("RpcThrottlingException_Type_NumRequestsExceeded_User_alice_Table_users"); + assertTrue("Counter metric should be present", metric.isPresent()); + assertTrue("Metric should be a counter", metric.get() instanceof Counter); + + Counter counter = (Counter) metric.get(); + assertEquals("Counter should have count of 1", 1, counter.getCount()); + } + + @Test + public void testMultipleThrottleTypes() { + // Create a test registry for the metrics + MetricRegistryInfo registryInfo = getRegistryInfo(); + MetricRegistry testRegistry = MetricRegistries.global().create(registryInfo); + MetricsThrottleExceptions throttleMetrics = new MetricsThrottleExceptions(testRegistry); + + // Record different types of throttle exceptions + throttleMetrics.recordThrottleException(RpcThrottlingException.Type.NumRequestsExceeded, + "alice", "users"); + throttleMetrics.recordThrottleException(RpcThrottlingException.Type.WriteSizeExceeded, "bob", + "logs"); + throttleMetrics.recordThrottleException(RpcThrottlingException.Type.ReadSizeExceeded, "charlie", + "metadata"); + + // Verify all three counters were created + verifyCounter(testRegistry, + "RpcThrottlingException_Type_NumRequestsExceeded_User_alice_Table_users", 1); + verifyCounter(testRegistry, "RpcThrottlingException_Type_WriteSizeExceeded_User_bob_Table_logs", + 1); + verifyCounter(testRegistry, + "RpcThrottlingException_Type_ReadSizeExceeded_User_charlie_Table_metadata", 1); + } + + @Test + public void testCounterIncrement() { + // Create a test registry for the metrics + MetricRegistryInfo registryInfo = getRegistryInfo(); + MetricRegistry testRegistry = MetricRegistries.global().create(registryInfo); + MetricsThrottleExceptions throttleMetrics = new MetricsThrottleExceptions(testRegistry); + + // Record the same throttle exception multiple times + String metricName = "RpcThrottlingException_Type_NumRequestsExceeded_User_alice_Table_users"; + throttleMetrics.recordThrottleException(RpcThrottlingException.Type.NumRequestsExceeded, + "alice", "users"); + throttleMetrics.recordThrottleException(RpcThrottlingException.Type.NumRequestsExceeded, + "alice", "users"); + throttleMetrics.recordThrottleException(RpcThrottlingException.Type.NumRequestsExceeded, + "alice", "users"); + + // Verify the counter incremented correctly + verifyCounter(testRegistry, metricName, 3); + } + + @Test + public void testMetricNameSanitization() { + // Create a test registry for the metrics + MetricRegistryInfo registryInfo = getRegistryInfo(); + MetricRegistry testRegistry = MetricRegistries.global().create(registryInfo); + MetricsThrottleExceptions throttleMetrics = new MetricsThrottleExceptions(testRegistry); + + // Test that meaningful characters are preserved (hyphens, periods, etc.) + throttleMetrics.recordThrottleException(RpcThrottlingException.Type.WriteSizeExceeded, + "user.name@company", "my-table-prod"); + + // Verify meaningful characters are preserved, only JMX-problematic chars are replaced + String expectedMetricName = + "RpcThrottlingException_Type_WriteSizeExceeded_User_user.name@company_Table_my-table-prod"; + verifyCounter(testRegistry, expectedMetricName, 1); + + // Test that JMX-problematic characters are sanitized + throttleMetrics.recordThrottleException(RpcThrottlingException.Type.ReadSizeExceeded, + "user,with=bad:chars*", "table?with\"quotes"); + String problematicMetricName = + "RpcThrottlingException_Type_ReadSizeExceeded_User_user_with_bad_chars__Table_table_with_quotes"; + verifyCounter(testRegistry, problematicMetricName, 1); + } + + @Test + public void testNullHandling() { + // Create a test registry for the metrics + MetricRegistryInfo registryInfo = getRegistryInfo(); + MetricRegistry testRegistry = MetricRegistries.global().create(registryInfo); + MetricsThrottleExceptions throttleMetrics = new MetricsThrottleExceptions(testRegistry); + + // Test null user and table names + throttleMetrics.recordThrottleException(RpcThrottlingException.Type.NumRequestsExceeded, null, + null); + throttleMetrics.recordThrottleException(RpcThrottlingException.Type.WriteSizeExceeded, "alice", + null); + throttleMetrics.recordThrottleException(RpcThrottlingException.Type.ReadSizeExceeded, null, + "users"); + + // Verify null values are replaced with "unknown" + verifyCounter(testRegistry, + "RpcThrottlingException_Type_NumRequestsExceeded_User_unknown_Table_unknown", 1); + verifyCounter(testRegistry, + "RpcThrottlingException_Type_WriteSizeExceeded_User_alice_Table_unknown", 1); + verifyCounter(testRegistry, + "RpcThrottlingException_Type_ReadSizeExceeded_User_unknown_Table_users", 1); + } + + @Test + public void testConcurrentAccess() throws InterruptedException { + // Create a test registry for the metrics + MetricRegistryInfo registryInfo = getRegistryInfo(); + MetricRegistry testRegistry = MetricRegistries.global().create(registryInfo); + MetricsThrottleExceptions throttleMetrics = new MetricsThrottleExceptions(testRegistry); + + int numThreads = 10; + int incrementsPerThread = 100; + + ExecutorService executor = Executors.newFixedThreadPool(numThreads); + CountDownLatch startLatch = new CountDownLatch(1); + CountDownLatch doneLatch = new CountDownLatch(numThreads); + AtomicInteger exceptions = new AtomicInteger(0); + + // Create multiple threads that increment the same counter concurrently + for (int i = 0; i < numThreads; i++) { + executor.submit(() -> { + try { + startLatch.await(); + for (int j = 0; j < incrementsPerThread; j++) { + throttleMetrics.recordThrottleException(RpcThrottlingException.Type.NumRequestsExceeded, + "alice", "users"); + } + } catch (Exception e) { + exceptions.incrementAndGet(); + } finally { + doneLatch.countDown(); + } + }); + } + + // Start all threads at once + startLatch.countDown(); + + // Wait for all threads to complete + boolean completed = doneLatch.await(30, TimeUnit.SECONDS); + assertTrue("All threads should complete within timeout", completed); + assertEquals("No exceptions should occur during concurrent access", 0, exceptions.get()); + + // Verify the final counter value + verifyCounter(testRegistry, + "RpcThrottlingException_Type_NumRequestsExceeded_User_alice_Table_users", + numThreads * incrementsPerThread); + + executor.shutdown(); + } + + @Test + public void testCommonTableNamePatterns() { + // Create a test registry for the metrics + MetricRegistryInfo registryInfo = getRegistryInfo(); + MetricRegistry testRegistry = MetricRegistries.global().create(registryInfo); + MetricsThrottleExceptions throttleMetrics = new MetricsThrottleExceptions(testRegistry); + + // Test common HBase table name patterns that should be preserved + throttleMetrics.recordThrottleException(RpcThrottlingException.Type.NumRequestsExceeded, + "service-user", "my-app-logs"); + throttleMetrics.recordThrottleException(RpcThrottlingException.Type.WriteSizeExceeded, + "batch.process", "namespace:table-name"); + throttleMetrics.recordThrottleException(RpcThrottlingException.Type.ReadSizeExceeded, + "user_123", "test_table_v2"); + + // Verify common patterns are preserved correctly (note: colon gets replaced with underscore) + verifyCounter(testRegistry, + "RpcThrottlingException_Type_NumRequestsExceeded_User_service-user_Table_my-app-logs", 1); + verifyCounter(testRegistry, + "RpcThrottlingException_Type_WriteSizeExceeded_User_batch.process_Table_namespace_table-name", + 1); + verifyCounter(testRegistry, + "RpcThrottlingException_Type_ReadSizeExceeded_User_user_123_Table_test_table_v2", 1); + } + + @Test + public void testAllThrottleExceptionTypes() { + // Create a test registry for the metrics + MetricRegistryInfo registryInfo = getRegistryInfo(); + MetricRegistry testRegistry = MetricRegistries.global().create(registryInfo); + MetricsThrottleExceptions throttleMetrics = new MetricsThrottleExceptions(testRegistry); + + // Test all 13 throttle exception types from RpcThrottlingException.Type enum + RpcThrottlingException.Type[] throttleTypes = { RpcThrottlingException.Type.NumRequestsExceeded, + RpcThrottlingException.Type.RequestSizeExceeded, + RpcThrottlingException.Type.NumReadRequestsExceeded, + RpcThrottlingException.Type.NumWriteRequestsExceeded, + RpcThrottlingException.Type.WriteSizeExceeded, RpcThrottlingException.Type.ReadSizeExceeded, + RpcThrottlingException.Type.RequestCapacityUnitExceeded, + RpcThrottlingException.Type.ReadCapacityUnitExceeded, + RpcThrottlingException.Type.WriteCapacityUnitExceeded, + RpcThrottlingException.Type.AtomicRequestNumberExceeded, + RpcThrottlingException.Type.AtomicReadSizeExceeded, + RpcThrottlingException.Type.AtomicWriteSizeExceeded, + RpcThrottlingException.Type.RequestHandlerUsageTimeExceeded }; Review Comment: What if `RpcThrottlingException` adds another enum type? This unit test wont cover the new type unless it's added here. Similarly, removing a type will break the test. You can use something like this do dynamically get the enum types? ```suggestion RpcThrottlingException.Type[] throttleTypes = RpcThrottlingException.Type.values(); ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
