From eb23af3fd13fb0f7c5f2abfea157c27dea7e05a0 Mon Sep 17 00:00:00 2001 From: vaijosh Date: Sat, 19 Nov 2022 13:12:33 +0530 Subject: [PATCH 1/7] HBASE-27498-Added a logic to cache master state so that all threads don't need to make expensive rpc calls in synchronize block. --- .../client/ConnectionImplementation.java | 55 ++++++++++++------- 1 file changed, 36 insertions(+), 19 deletions(-) diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionImplementation.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionImplementation.java index b66e4242b3a3..580dff266e5f 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionImplementation.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionImplementation.java @@ -89,6 +89,8 @@ import org.apache.hadoop.hbase.util.Threads; import org.apache.hadoop.ipc.RemoteException; import org.apache.hadoop.security.UserGroupInformation; +import org.apache.hbase.thirdparty.com.google.common.base.Supplier; +import org.apache.hbase.thirdparty.com.google.common.base.Suppliers; import org.apache.yetus.audience.InterfaceAudience; import org.apache.zookeeper.KeeperException; import org.slf4j.Logger; @@ -169,6 +171,9 @@ @InterfaceAudience.Private public class ConnectionImplementation implements ClusterConnection, Closeable { public static final String RETRIES_BY_SERVER_KEY = "hbase.client.retries.by.server"; + + public static final String + MASTER_STATE_CACHE_TIMEOUT_SEC = "hbase.client.master.state.cache.timeout.sec"; private static final Logger LOG = LoggerFactory.getLogger(ConnectionImplementation.class); // The mode tells if HedgedRead, LoadBalance mode is supported. @@ -249,6 +254,10 @@ public class ConnectionImplementation implements ClusterConnection, Closeable { /** lock guards against multiple threads trying to query the meta region at the same time */ private final ReentrantLock userRegionLock = new ReentrantLock(); + + /** A Guava based TTL Cache to cache state of the master i.e. master is running or not */ + final Supplier cachedMasterStateSupplier; + private ChoreService choreService; /** @@ -382,6 +391,24 @@ public void newDead(ServerName sn) { default: // Doing nothing } + + this.cachedMasterStateSupplier = Suppliers.memoizeWithExpiration(() -> { + if (this.masterServiceState.getStub() == null) { + return false; + } + try { + LOG.info("Getting master state using rpc call"); + return this.masterServiceState.isMasterRunning(); + } catch (UndeclaredThrowableException e) { + // It's somehow messy, but we can receive exceptions such as + // java.net.ConnectException but they're not declared. So we catch it... + LOG.info("Master connection is not running anymore", e.getUndeclaredThrowable()); + return false; + } catch (IOException se) { + LOG.warn("Checking master connection", se); + return false; + } + }, conf.getLong(MASTER_STATE_CACHE_TIMEOUT_SEC, 30), TimeUnit.SECONDS); } private void spawnRenewalChore(final UserGroupInformation user) { @@ -1365,12 +1392,14 @@ private void resetMasterServiceState(final MasterServiceState mss) { } private MasterKeepAliveConnection getKeepAliveMasterService() throws IOException { - synchronized (masterLock) { - if (!isKeepAliveMasterConnectedAndRunning(this.masterServiceState)) { - MasterServiceStubMaker stubMaker = new MasterServiceStubMaker(); - this.masterServiceState.stub = stubMaker.makeStub(); + if(!isKeepAliveMasterConnectedAndRunning(this.masterServiceState)){ + synchronized (masterLock) { + if (!isKeepAliveMasterConnectedAndRunning(this.masterServiceState)) { + MasterServiceStubMaker stubMaker = new MasterServiceStubMaker(); + this.masterServiceState.stub = stubMaker.makeStub(); + } + resetMasterServiceState(this.masterServiceState); } - resetMasterServiceState(this.masterServiceState); } // Ugly delegation just so we can add in a Close method. final MasterProtos.MasterService.BlockingInterface stub = this.masterServiceState.stub; @@ -1945,20 +1974,8 @@ private static void release(MasterServiceState mss) { } private boolean isKeepAliveMasterConnectedAndRunning(MasterServiceState mss) { - if (mss.getStub() == null) { - return false; - } - try { - return mss.isMasterRunning(); - } catch (UndeclaredThrowableException e) { - // It's somehow messy, but we can receive exceptions such as - // java.net.ConnectException but they're not declared. So we catch it... - LOG.info("Master connection is not running anymore", e.getUndeclaredThrowable()); - return false; - } catch (IOException se) { - LOG.warn("Checking master connection", se); - return false; - } + LOG.info("Getting master connection state from TTL Cache"); + return cachedMasterStateSupplier.get(); } void releaseMaster(MasterServiceState mss) { From e3700d1cd89e9e75c10921e0161914edf365326c Mon Sep 17 00:00:00 2001 From: vaijosh Date: Sat, 19 Nov 2022 14:15:32 +0530 Subject: [PATCH 2/7] HBASE-27498-Fixed formatting errors using "mvn spotless:apply" --- .../hadoop/hbase/client/ConnectionImplementation.java | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionImplementation.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionImplementation.java index 580dff266e5f..583dbe3c737f 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionImplementation.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionImplementation.java @@ -89,13 +89,13 @@ import org.apache.hadoop.hbase.util.Threads; import org.apache.hadoop.ipc.RemoteException; import org.apache.hadoop.security.UserGroupInformation; -import org.apache.hbase.thirdparty.com.google.common.base.Supplier; -import org.apache.hbase.thirdparty.com.google.common.base.Suppliers; import org.apache.yetus.audience.InterfaceAudience; import org.apache.zookeeper.KeeperException; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import org.apache.hbase.thirdparty.com.google.common.base.Supplier; +import org.apache.hbase.thirdparty.com.google.common.base.Suppliers; import org.apache.hbase.thirdparty.com.google.common.base.Throwables; import org.apache.hbase.thirdparty.com.google.common.util.concurrent.ThreadFactoryBuilder; import org.apache.hbase.thirdparty.com.google.protobuf.BlockingRpcChannel; @@ -172,8 +172,8 @@ public class ConnectionImplementation implements ClusterConnection, Closeable { public static final String RETRIES_BY_SERVER_KEY = "hbase.client.retries.by.server"; - public static final String - MASTER_STATE_CACHE_TIMEOUT_SEC = "hbase.client.master.state.cache.timeout.sec"; + public static final String MASTER_STATE_CACHE_TIMEOUT_SEC = + "hbase.client.master.state.cache.timeout.sec"; private static final Logger LOG = LoggerFactory.getLogger(ConnectionImplementation.class); // The mode tells if HedgedRead, LoadBalance mode is supported. @@ -254,7 +254,6 @@ public class ConnectionImplementation implements ClusterConnection, Closeable { /** lock guards against multiple threads trying to query the meta region at the same time */ private final ReentrantLock userRegionLock = new ReentrantLock(); - /** A Guava based TTL Cache to cache state of the master i.e. master is running or not */ final Supplier cachedMasterStateSupplier; @@ -1392,7 +1391,7 @@ private void resetMasterServiceState(final MasterServiceState mss) { } private MasterKeepAliveConnection getKeepAliveMasterService() throws IOException { - if(!isKeepAliveMasterConnectedAndRunning(this.masterServiceState)){ + if (!isKeepAliveMasterConnectedAndRunning(this.masterServiceState)) { synchronized (masterLock) { if (!isKeepAliveMasterConnectedAndRunning(this.masterServiceState)) { MasterServiceStubMaker stubMaker = new MasterServiceStubMaker(); From f90f0f28e39b03e17ccdc30640729fddea570635 Mon Sep 17 00:00:00 2001 From: vaijosh Date: Fri, 25 Nov 2022 11:58:40 +0530 Subject: [PATCH 3/7] HBASE-27498 : Implemented Review comments 1.Default master state cache timeout is 0 i.e. legacy behavior. 2.Added integration test around new property --- .../client/ConnectionImplementation.java | 31 ++++-- .../client/TestConnectionImplementation.java | 96 +++++++++++++++++++ 2 files changed, 121 insertions(+), 6 deletions(-) create mode 100644 hbase-it/src/test/java/org/apache/hadoop/hbase/client/TestConnectionImplementation.java diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionImplementation.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionImplementation.java index 583dbe3c737f..7bed5ea4c92d 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionImplementation.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionImplementation.java @@ -254,8 +254,12 @@ public class ConnectionImplementation implements ClusterConnection, Closeable { /** lock guards against multiple threads trying to query the meta region at the same time */ private final ReentrantLock userRegionLock = new ReentrantLock(); - /** A Guava based TTL Cache to cache state of the master i.e. master is running or not */ - final Supplier cachedMasterStateSupplier; + /** + * Supplier to get masterState + * - By default uses simple supplier without TTL cache + * - Use TTL Cache when hbase.client.master.state.cache.timeout.sec > 0 + */ + private final Supplier masterStateSupplier; private ChoreService choreService; @@ -391,7 +395,22 @@ public void newDead(ServerName sn) { // Doing nothing } - this.cachedMasterStateSupplier = Suppliers.memoizeWithExpiration(() -> { + long masterStateCacheTimeout = conf.getLong(MASTER_STATE_CACHE_TIMEOUT_SEC, 0); + + Supplier masterConnSupplier = masterConnectionStateSupplier(); + if (masterStateCacheTimeout <= 0L) { + this.masterStateSupplier = masterConnSupplier; + } else { + this.masterStateSupplier = Suppliers.memoizeWithExpiration(masterConnSupplier, + masterStateCacheTimeout, TimeUnit.SECONDS); + } + } + + /** + * Visible for tests + */ + Supplier masterConnectionStateSupplier() { + return () -> { if (this.masterServiceState.getStub() == null) { return false; } @@ -407,7 +426,7 @@ public void newDead(ServerName sn) { LOG.warn("Checking master connection", se); return false; } - }, conf.getLong(MASTER_STATE_CACHE_TIMEOUT_SEC, 30), TimeUnit.SECONDS); + }; } private void spawnRenewalChore(final UserGroupInformation user) { @@ -1281,7 +1300,6 @@ public void addError() { * Class to make a MasterServiceStubMaker stub. */ private final class MasterServiceStubMaker { - private void isMasterRunning(MasterProtos.MasterService.BlockingInterface stub) throws IOException { try { @@ -1400,6 +1418,7 @@ private MasterKeepAliveConnection getKeepAliveMasterService() throws IOException resetMasterServiceState(this.masterServiceState); } } + // Ugly delegation just so we can add in a Close method. final MasterProtos.MasterService.BlockingInterface stub = this.masterServiceState.stub; return new MasterKeepAliveConnection() { @@ -1974,7 +1993,7 @@ private static void release(MasterServiceState mss) { private boolean isKeepAliveMasterConnectedAndRunning(MasterServiceState mss) { LOG.info("Getting master connection state from TTL Cache"); - return cachedMasterStateSupplier.get(); + return masterStateSupplier.get(); } void releaseMaster(MasterServiceState mss) { diff --git a/hbase-it/src/test/java/org/apache/hadoop/hbase/client/TestConnectionImplementation.java b/hbase-it/src/test/java/org/apache/hadoop/hbase/client/TestConnectionImplementation.java new file mode 100644 index 000000000000..4a57b9efecec --- /dev/null +++ b/hbase-it/src/test/java/org/apache/hadoop/hbase/client/TestConnectionImplementation.java @@ -0,0 +1,96 @@ +/* + * 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 java.io.IOException; +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.hbase.HBaseClassTestRule; +import org.apache.hadoop.hbase.IntegrationTestingUtility; +import org.apache.hadoop.hbase.security.UserProvider; +import org.apache.hadoop.hbase.testclassification.ClientTests; +import org.apache.hadoop.hbase.testclassification.MediumTests; +import org.junit.AfterClass; +import org.junit.BeforeClass; +import org.junit.ClassRule; +import org.junit.Test; +import org.junit.experimental.categories.Category; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +@Category({ ClientTests.class, MediumTests.class }) +public class TestConnectionImplementation { + @ClassRule + public static final HBaseClassTestRule CLASS_RULE = + HBaseClassTestRule.forClass(TestConnectionImplementation.class); + + private static final Logger LOG = LoggerFactory.getLogger(TestConnectionImplementation.class); + + private static final IntegrationTestingUtility TEST_UTIL = new IntegrationTestingUtility(); + + @BeforeClass + public static void beforeClass() throws Exception { + TEST_UTIL.startMiniCluster(1); + } + + @AfterClass + public static void afterClass() throws Exception { + TEST_UTIL.shutdownMiniCluster(); + } + + @Test + public void testGetMasterTTLCache() throws IOException, InterruptedException { + Configuration conf = TEST_UTIL.getConfiguration(); + + // Test with hbase.client.master.state.cache.timeout.sec = 0 sec + conf.setLong(ConnectionImplementation.MASTER_STATE_CACHE_TIMEOUT_SEC, 0L); + ConnectionImplementation conn = + new ConnectionImplementation(conf, null, UserProvider.instantiate(conf).getCurrent()); + long startTime = System.currentTimeMillis(); + conn.getMaster(); + long endTime = System.currentTimeMillis(); + long totalMillisWithoutCache = endTime - startTime; + + LOG.info("TotalMillisWithoutCache:{} ms", totalMillisWithoutCache); + + + // Test with hbase.client.master.state.cache.timeout.sec = 15 Sec + conf.setLong(ConnectionImplementation.MASTER_STATE_CACHE_TIMEOUT_SEC, 15L); + conn = new ConnectionImplementation(conf, null, UserProvider.instantiate(conf).getCurrent()); + conn.getMaster(); + + startTime = System.currentTimeMillis(); + conn.getMaster(); + endTime = System.currentTimeMillis(); + long totalMillisWithCache = endTime - startTime; + + LOG.info("totalMillisWithCache:{} ms", totalMillisWithCache); + + Thread.sleep(20000); + startTime = System.currentTimeMillis(); + conn.getMaster(); + endTime = System.currentTimeMillis(); + long totalMillisAfterCacheExpiry = endTime - startTime; + LOG.info("totalMillisAfterCacheExpiry:{} ms", totalMillisAfterCacheExpiry); + + // Verify that retrieval from cache is faster than retrieval without cache. + assert totalMillisWithCache < totalMillisWithoutCache; + + // Verify that retrieval from cache is faster than retrieval after expiry. + assert totalMillisWithCache < totalMillisAfterCacheExpiry; + } +} From 9b2ef31051fb70c325fd10b74f5d1ba182faccb2 Mon Sep 17 00:00:00 2001 From: vaijosh Date: Fri, 25 Nov 2022 12:56:56 +0530 Subject: [PATCH 4/7] HBASE-27498 : Fixed Spotless formatting issue. --- .../apache/hadoop/hbase/client/ConnectionImplementation.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionImplementation.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionImplementation.java index 7bed5ea4c92d..3fa164ffc854 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionImplementation.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionImplementation.java @@ -255,9 +255,8 @@ public class ConnectionImplementation implements ClusterConnection, Closeable { private final ReentrantLock userRegionLock = new ReentrantLock(); /** - * Supplier to get masterState - * - By default uses simple supplier without TTL cache - * - Use TTL Cache when hbase.client.master.state.cache.timeout.sec > 0 + * Supplier to get masterState.By default uses simple supplier without TTL cache. When + * hbase.client.master.state.cache.timeout.sec > 0 it uses TTL Cache. */ private final Supplier masterStateSupplier; From be30d7c2844e55d9ae6ca4bdfaad7dc6cd062d37 Mon Sep 17 00:00:00 2001 From: vaijosh Date: Fri, 25 Nov 2022 13:40:25 +0530 Subject: [PATCH 5/7] HBASE-27498 : Fixed Spotless formatting issue. --- .../apache/hadoop/hbase/client/TestConnectionImplementation.java | 1 - 1 file changed, 1 deletion(-) diff --git a/hbase-it/src/test/java/org/apache/hadoop/hbase/client/TestConnectionImplementation.java b/hbase-it/src/test/java/org/apache/hadoop/hbase/client/TestConnectionImplementation.java index 4a57b9efecec..7ed22d29c3b9 100644 --- a/hbase-it/src/test/java/org/apache/hadoop/hbase/client/TestConnectionImplementation.java +++ b/hbase-it/src/test/java/org/apache/hadoop/hbase/client/TestConnectionImplementation.java @@ -67,7 +67,6 @@ public void testGetMasterTTLCache() throws IOException, InterruptedException { LOG.info("TotalMillisWithoutCache:{} ms", totalMillisWithoutCache); - // Test with hbase.client.master.state.cache.timeout.sec = 15 Sec conf.setLong(ConnectionImplementation.MASTER_STATE_CACHE_TIMEOUT_SEC, 15L); conn = new ConnectionImplementation(conf, null, UserProvider.instantiate(conf).getCurrent()); From 467395efacda6724dc04c13798ca16e37d3062b4 Mon Sep 17 00:00:00 2001 From: vaijosh Date: Fri, 2 Dec 2022 19:08:40 +0530 Subject: [PATCH 6/7] HBASE-27498 : Fixed flaky test TestConnectionImplementation --- .../client/TestConnectionImplementation.java | 127 +++++++++++++----- 1 file changed, 96 insertions(+), 31 deletions(-) diff --git a/hbase-it/src/test/java/org/apache/hadoop/hbase/client/TestConnectionImplementation.java b/hbase-it/src/test/java/org/apache/hadoop/hbase/client/TestConnectionImplementation.java index 7ed22d29c3b9..6b721083bbc1 100644 --- a/hbase-it/src/test/java/org/apache/hadoop/hbase/client/TestConnectionImplementation.java +++ b/hbase-it/src/test/java/org/apache/hadoop/hbase/client/TestConnectionImplementation.java @@ -18,6 +18,10 @@ package org.apache.hadoop.hbase.client; import java.io.IOException; +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Method; +import java.lang.reflect.UndeclaredThrowableException; +import org.apache.commons.lang3.reflect.FieldUtils; import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.hbase.HBaseClassTestRule; import org.apache.hadoop.hbase.IntegrationTestingUtility; @@ -25,21 +29,21 @@ import org.apache.hadoop.hbase.testclassification.ClientTests; import org.apache.hadoop.hbase.testclassification.MediumTests; import org.junit.AfterClass; +import org.junit.Assert; import org.junit.BeforeClass; import org.junit.ClassRule; import org.junit.Test; import org.junit.experimental.categories.Category; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; +import org.junit.runner.RunWith; +import org.mockito.Mockito; +import org.mockito.junit.MockitoJUnitRunner; @Category({ ClientTests.class, MediumTests.class }) +@RunWith(MockitoJUnitRunner.class) public class TestConnectionImplementation { @ClassRule public static final HBaseClassTestRule CLASS_RULE = HBaseClassTestRule.forClass(TestConnectionImplementation.class); - - private static final Logger LOG = LoggerFactory.getLogger(TestConnectionImplementation.class); - private static final IntegrationTestingUtility TEST_UTIL = new IntegrationTestingUtility(); @BeforeClass @@ -53,43 +57,104 @@ public static void afterClass() throws Exception { } @Test - public void testGetMasterTTLCache() throws IOException, InterruptedException { + public void testGetMaster_noCachedMasterState() throws IOException, IllegalAccessException { Configuration conf = TEST_UTIL.getConfiguration(); - - // Test with hbase.client.master.state.cache.timeout.sec = 0 sec conf.setLong(ConnectionImplementation.MASTER_STATE_CACHE_TIMEOUT_SEC, 0L); ConnectionImplementation conn = new ConnectionImplementation(conf, null, UserProvider.instantiate(conf).getCurrent()); - long startTime = System.currentTimeMillis(); - conn.getMaster(); - long endTime = System.currentTimeMillis(); - long totalMillisWithoutCache = endTime - startTime; - - LOG.info("TotalMillisWithoutCache:{} ms", totalMillisWithoutCache); + ConnectionImplementation.MasterServiceState masterServiceState = spyMasterServiceState(conn); + conn.getMaster(); // This initializes the stubs but don't call isMasterRunning + conn.getMaster(); // Calls isMasterRunning since stubs are initialized. Invocation 1 + conn.getMaster(); // Calls isMasterRunning since stubs are initialized. Invocation 2 + Mockito.verify(masterServiceState, Mockito.times(2)).isMasterRunning(); + conn.close(); + } - // Test with hbase.client.master.state.cache.timeout.sec = 15 Sec + @Test + public void testGetMaster_masterStateCacheHit() throws IOException, IllegalAccessException { + Configuration conf = TEST_UTIL.getConfiguration(); conf.setLong(ConnectionImplementation.MASTER_STATE_CACHE_TIMEOUT_SEC, 15L); - conn = new ConnectionImplementation(conf, null, UserProvider.instantiate(conf).getCurrent()); - conn.getMaster(); + ConnectionImplementation conn = + new ConnectionImplementation(conf, null, UserProvider.instantiate(conf).getCurrent()); + ConnectionImplementation.MasterServiceState masterServiceState = spyMasterServiceState(conn); + conn.getMaster(); // This initializes the stubs but don't call isMasterRunning + conn.getMaster(); // Uses cached value, don't call isMasterRunning + conn.getMaster(); // Uses cached value, don't call isMasterRunning + Mockito.verify(masterServiceState, Mockito.times(0)).isMasterRunning(); + conn.close(); + } - startTime = System.currentTimeMillis(); - conn.getMaster(); - endTime = System.currentTimeMillis(); - long totalMillisWithCache = endTime - startTime; + @Test + public void testGetMaster_masterStateCacheMiss() + throws IOException, InterruptedException, IllegalAccessException { + Configuration conf = TEST_UTIL.getConfiguration(); + conf.setLong(ConnectionImplementation.MASTER_STATE_CACHE_TIMEOUT_SEC, 5L); + ConnectionImplementation conn = + new ConnectionImplementation(conf, null, UserProvider.instantiate(conf).getCurrent()); + ConnectionImplementation.MasterServiceState masterServiceState = spyMasterServiceState(conn); + conn.getMaster(); // This initializes the stubs but don't call isMasterRunning + conn.getMaster(); // Uses cached value, don't call isMasterRunning + conn.getMaster(); // Uses cached value, don't call isMasterRunning + Thread.sleep(10000); + conn.getMaster(); // Calls isMasterRunning after cache expiry + Mockito.verify(masterServiceState, Mockito.times(1)).isMasterRunning(); + conn.close(); + } - LOG.info("totalMillisWithCache:{} ms", totalMillisWithCache); + @Test + public void testIsKeepAliveMasterConnectedAndRunning_UndeclaredThrowableException() + throws IOException, IllegalAccessException, NoSuchMethodException, InvocationTargetException { + Configuration conf = TEST_UTIL.getConfiguration(); + conf.setLong(ConnectionImplementation.MASTER_STATE_CACHE_TIMEOUT_SEC, 0); + ConnectionImplementation conn = + new ConnectionImplementation(conf, null, UserProvider.instantiate(conf).getCurrent()); + conn.getMaster(); // Initializes stubs - Thread.sleep(20000); - startTime = System.currentTimeMillis(); + ConnectionImplementation.MasterServiceState masterServiceState = spyMasterServiceState(conn); + Mockito.doThrow(new UndeclaredThrowableException(new Exception("DUMMY EXCEPTION"))) + .when(masterServiceState).isMasterRunning(); + + // Verify that masterState is "false" because of to injected exception + boolean isKeepAliveMasterRunning = + (boolean) getIsKeepAliveMasterConnectedAndRunningMethod().invoke(conn); + Assert.assertFalse(isKeepAliveMasterRunning); + conn.close(); + } + + @Test + public void testIsKeepAliveMasterConnectedAndRunning_IOException() + throws IOException, IllegalAccessException, NoSuchMethodException, InvocationTargetException { + Configuration conf = TEST_UTIL.getConfiguration(); + conf.setLong(ConnectionImplementation.MASTER_STATE_CACHE_TIMEOUT_SEC, 0); + ConnectionImplementation conn = + new ConnectionImplementation(conf, null, UserProvider.instantiate(conf).getCurrent()); conn.getMaster(); - endTime = System.currentTimeMillis(); - long totalMillisAfterCacheExpiry = endTime - startTime; - LOG.info("totalMillisAfterCacheExpiry:{} ms", totalMillisAfterCacheExpiry); - // Verify that retrieval from cache is faster than retrieval without cache. - assert totalMillisWithCache < totalMillisWithoutCache; + ConnectionImplementation.MasterServiceState masterServiceState = spyMasterServiceState(conn); + Mockito.doThrow(new IOException("DUMMY EXCEPTION")).when(masterServiceState).isMasterRunning(); + + boolean isKeepAliveMasterRunning = + (boolean) getIsKeepAliveMasterConnectedAndRunningMethod().invoke(conn); + + // Verify that masterState is "false" because of to injected exception + Assert.assertFalse(isKeepAliveMasterRunning); + conn.close(); + } + + // Spy the masterServiceState object using reflection + private ConnectionImplementation.MasterServiceState + spyMasterServiceState(ConnectionImplementation conn) throws IllegalAccessException { + ConnectionImplementation.MasterServiceState spiedMasterServiceState = + Mockito.spy(conn.getMasterServiceState()); + FieldUtils.writeDeclaredField(conn, "masterServiceState", spiedMasterServiceState, true); + return spiedMasterServiceState; + } - // Verify that retrieval from cache is faster than retrieval after expiry. - assert totalMillisWithCache < totalMillisAfterCacheExpiry; + // Get isKeepAliveMasterConnectedAndRunning using reflection + private Method getIsKeepAliveMasterConnectedAndRunningMethod() throws NoSuchMethodException { + Method method = + ConnectionImplementation.class.getDeclaredMethod("isKeepAliveMasterConnectedAndRunning"); + method.setAccessible(true); + return method; } } From fb56dbaec0def05047a92f3d50ba997109962d5e Mon Sep 17 00:00:00 2001 From: vaijosh Date: Fri, 2 Dec 2022 19:24:45 +0530 Subject: [PATCH 7/7] HBASE-27498 : Minor changes to fix flaky test --- .../hbase/client/ConnectionImplementation.java | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionImplementation.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionImplementation.java index 3fa164ffc854..a4d24777d37f 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionImplementation.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionImplementation.java @@ -1398,6 +1398,13 @@ public BlockingInterface getClient(ServerName serverName) throws IOException { final MasterServiceState masterServiceState = new MasterServiceState(this); + /** + * Visible for tests + */ + MasterServiceState getMasterServiceState() { + return this.masterServiceState; + } + @Override public MasterKeepAliveConnection getMaster() throws IOException { return getKeepAliveMasterService(); @@ -1408,9 +1415,9 @@ private void resetMasterServiceState(final MasterServiceState mss) { } private MasterKeepAliveConnection getKeepAliveMasterService() throws IOException { - if (!isKeepAliveMasterConnectedAndRunning(this.masterServiceState)) { + if (!isKeepAliveMasterConnectedAndRunning()) { synchronized (masterLock) { - if (!isKeepAliveMasterConnectedAndRunning(this.masterServiceState)) { + if (!isKeepAliveMasterConnectedAndRunning()) { MasterServiceStubMaker stubMaker = new MasterServiceStubMaker(); this.masterServiceState.stub = stubMaker.makeStub(); } @@ -1990,7 +1997,7 @@ private static void release(MasterServiceState mss) { } } - private boolean isKeepAliveMasterConnectedAndRunning(MasterServiceState mss) { + private boolean isKeepAliveMasterConnectedAndRunning() { LOG.info("Getting master connection state from TTL Cache"); return masterStateSupplier.get(); }