From 74b28a8ca8eb61cee0f5d06faa310110b9e7a6dd Mon Sep 17 00:00:00 2001 From: Keith Turner Date: Fri, 4 Oct 2024 14:25:46 +0000 Subject: [PATCH] avoids uneeded object allocation on ServiceLocksPath ServiceLockPaths allows filtering hosts using a predicate. In the case where a predicate was passed in that always returned true a HostPort object needlessly allocated to pass to the predicate. These changes refactor the predicate to only allocate the HostAndPort object when needed. Before the changes in #4943 this was the behavior, that when all host were wanted no objects were allocated. --- .../core/clientImpl/ZookeeperLockChecker.java | 7 +++-- .../accumulo/core/lock/ServiceLockPaths.java | 21 ++++++++++---- .../core/lock/ServiceLockPathsTest.java | 28 +++++++++---------- .../server/manager/LiveTServerSet.java | 7 ++--- .../apache/accumulo/server/util/Admin.java | 5 ++-- .../server/util/TabletServerLocks.java | 3 +- 6 files changed, 40 insertions(+), 31 deletions(-) diff --git a/core/src/main/java/org/apache/accumulo/core/clientImpl/ZookeeperLockChecker.java b/core/src/main/java/org/apache/accumulo/core/clientImpl/ZookeeperLockChecker.java index 72e4e15d772..2f0367c102d 100644 --- a/core/src/main/java/org/apache/accumulo/core/clientImpl/ZookeeperLockChecker.java +++ b/core/src/main/java/org/apache/accumulo/core/clientImpl/ZookeeperLockChecker.java @@ -22,6 +22,7 @@ import org.apache.accumulo.core.clientImpl.ClientTabletCacheImpl.TabletServerLockChecker; import org.apache.accumulo.core.lock.ServiceLock; +import org.apache.accumulo.core.lock.ServiceLockPaths.AddressPredicate; import org.apache.accumulo.core.lock.ServiceLockPaths.ServiceLockPath; import com.google.common.net.HostAndPort; @@ -38,7 +39,7 @@ public boolean doesTabletServerLockExist(String server) { // ServiceLockPaths only returns items that have a lock var hostAndPort = HostAndPort.fromString(server); Set tservers = - ctx.getServerPaths().getTabletServer(rg -> true, addr -> addr.equals(hostAndPort), true); + ctx.getServerPaths().getTabletServer(rg -> true, AddressPredicate.exact(hostAndPort), true); return !tservers.isEmpty(); } @@ -47,7 +48,7 @@ public boolean isLockHeld(String server, String session) { // ServiceLockPaths only returns items that have a lock var hostAndPort = HostAndPort.fromString(server); Set tservers = - ctx.getServerPaths().getTabletServer(rg -> true, addr -> addr.equals(hostAndPort), true); + ctx.getServerPaths().getTabletServer(rg -> true, AddressPredicate.exact(hostAndPort), true); for (ServiceLockPath slp : tservers) { if (ServiceLock.getSessionId(ctx.getZooCache(), slp) == Long.parseLong(session, 16)) { return true; @@ -59,7 +60,7 @@ public boolean isLockHeld(String server, String session) { @Override public void invalidateCache(String tserver) { var hostAndPort = HostAndPort.fromString(tserver); - ctx.getServerPaths().getTabletServer(rg -> true, addr -> addr.equals(hostAndPort), false) + ctx.getServerPaths().getTabletServer(rg -> true, AddressPredicate.exact(hostAndPort), false) .forEach(slp -> { ctx.getZooCache().clear(slp.toString()); }); diff --git a/core/src/main/java/org/apache/accumulo/core/lock/ServiceLockPaths.java b/core/src/main/java/org/apache/accumulo/core/lock/ServiceLockPaths.java index 30c94e9ab44..b60712e92b9 100644 --- a/core/src/main/java/org/apache/accumulo/core/lock/ServiceLockPaths.java +++ b/core/src/main/java/org/apache/accumulo/core/lock/ServiceLockPaths.java @@ -284,7 +284,7 @@ public ServiceLockPath createDeadTabletServerPath(String resourceGroup, } public Set getCompactor(ResourceGroupPredicate resourceGroupPredicate, - Predicate address, boolean withLock) { + AddressPredicate address, boolean withLock) { return get(Constants.ZCOMPACTORS, resourceGroupPredicate, address, withLock); } @@ -316,17 +316,17 @@ public ServiceLockPath getMonitor(boolean withLock) { } public Set getScanServer(ResourceGroupPredicate resourceGroupPredicate, - Predicate address, boolean withLock) { + AddressPredicate address, boolean withLock) { return get(Constants.ZSSERVERS, resourceGroupPredicate, address, withLock); } public Set getTabletServer(ResourceGroupPredicate resourceGroupPredicate, - Predicate address, boolean withLock) { + AddressPredicate address, boolean withLock) { return get(Constants.ZTSERVERS, resourceGroupPredicate, address, withLock); } public Set getDeadTabletServer(ResourceGroupPredicate resourceGroupPredicate, - Predicate address, boolean withLock) { + AddressPredicate address, boolean withLock) { return get(Constants.ZDEADTSERVERS, resourceGroupPredicate, address, withLock); } @@ -334,6 +334,15 @@ public interface ResourceGroupPredicate extends Predicate { } + public interface AddressPredicate extends Predicate { + + static AddressPredicate exact(HostAndPort hostAndPort) { + Objects.requireNonNull(hostAndPort); + AddressPredicate predicate = addr -> hostAndPort.equals(HostAndPort.fromString(addr)); + return predicate; + } + } + /** * Find paths in ZooKeeper based on the input arguments and return a set of ServiceLockPath * objects. @@ -347,7 +356,7 @@ public interface ResourceGroupPredicate extends Predicate { * @return set of ServiceLockPath objects for the paths found based on the search criteria */ private Set get(final String serverType, - ResourceGroupPredicate resourceGroupPredicate, Predicate addressPredicate, + ResourceGroupPredicate resourceGroupPredicate, AddressPredicate addressPredicate, boolean withLock) { Objects.requireNonNull(serverType); @@ -380,7 +389,7 @@ private Set get(final String serverType, final ZcStat stat = new ZcStat(); final ServiceLockPath slp = parse(Optional.of(serverType), typePath + "/" + group + "/" + server); - if (addressPredicate.test(HostAndPort.fromString(server))) { + if (addressPredicate.test(server)) { if (!withLock || slp.getType().equals(Constants.ZDEADTSERVERS)) { // Dead TServers don't have lock data results.add(slp); diff --git a/core/src/test/java/org/apache/accumulo/core/lock/ServiceLockPathsTest.java b/core/src/test/java/org/apache/accumulo/core/lock/ServiceLockPathsTest.java index 3f91c756b7a..27e1e795a26 100644 --- a/core/src/test/java/org/apache/accumulo/core/lock/ServiceLockPathsTest.java +++ b/core/src/test/java/org/apache/accumulo/core/lock/ServiceLockPathsTest.java @@ -45,6 +45,7 @@ import org.apache.accumulo.core.fate.zookeeper.ZooCache; import org.apache.accumulo.core.fate.zookeeper.ZooCache.ZcStat; import org.apache.accumulo.core.lock.ServiceLockData.ThriftService; +import org.apache.accumulo.core.lock.ServiceLockPaths.AddressPredicate; import org.apache.accumulo.core.lock.ServiceLockPaths.ServiceLockPath; import org.easymock.EasyMock; import org.junit.jupiter.api.Test; @@ -385,7 +386,7 @@ public void testGetCompactorsNotRunning() { assertTrue(ctx.getServerPaths() .getCompactor(rg -> rg.equals(TEST_RESOURCE_GROUP), addr -> true, true).isEmpty()); assertTrue(ctx.getServerPaths() - .getCompactor(rg -> rg.equals(TEST_RESOURCE_GROUP), addr -> addr.equals(hp), true) + .getCompactor(rg -> rg.equals(TEST_RESOURCE_GROUP), AddressPredicate.exact(hp), true) .isEmpty()); EasyMock.verify(ctx, zc); @@ -504,7 +505,7 @@ public void testGetCompactors() { // query for a specific server results = ctx.getServerPaths().getCompactor(rg -> rg.equals(TEST_RESOURCE_GROUP), - addr -> addr.equals(hp), true); + AddressPredicate.exact(hp), true); assertEquals(1, results.size()); iter = results.iterator(); slp1 = iter.next(); @@ -515,7 +516,7 @@ public void testGetCompactors() { // query for a wrong server results = ctx.getServerPaths().getCompactor(rg -> rg.equals(TEST_RESOURCE_GROUP), - addr -> addr.equals(HostAndPort.fromString("localhost:1234")), true); + AddressPredicate.exact(HostAndPort.fromString("localhost:1234")), true); assertEquals(0, results.size()); EasyMock.verify(ctx, zc); @@ -542,7 +543,7 @@ public void testGetScanServersNotRunning() { assertTrue(ctx.getServerPaths() .getScanServer(rg -> rg.equals(TEST_RESOURCE_GROUP), addr -> true, true).isEmpty()); assertTrue(ctx.getServerPaths() - .getScanServer(rg -> rg.equals(TEST_RESOURCE_GROUP), addr -> addr.equals(hp), true) + .getScanServer(rg -> rg.equals(TEST_RESOURCE_GROUP), AddressPredicate.exact(hp), true) .isEmpty()); EasyMock.verify(ctx, zc); @@ -658,7 +659,7 @@ public void testGetScanServers() { // query for a specific server results = ctx.getServerPaths().getScanServer(rg -> rg.equals(TEST_RESOURCE_GROUP), - addr -> addr.equals(hp), true); + AddressPredicate.exact(hp), true); assertEquals(1, results.size()); iter = results.iterator(); slp1 = iter.next(); @@ -669,7 +670,7 @@ public void testGetScanServers() { // query for a wrong server results = ctx.getServerPaths().getScanServer(rg -> rg.equals(TEST_RESOURCE_GROUP), - addr -> addr.equals(HostAndPort.fromString("localhost:1234")), true); + AddressPredicate.exact(HostAndPort.fromString("localhost:1234")), true); assertEquals(0, results.size()); EasyMock.verify(ctx, zc); @@ -696,7 +697,7 @@ public void testGetTabletServersNotRunning() { assertTrue(ctx.getServerPaths() .getTabletServer(rg -> rg.equals(TEST_RESOURCE_GROUP), addr -> true, true).isEmpty()); assertTrue(ctx.getServerPaths() - .getTabletServer(rg -> rg.equals(TEST_RESOURCE_GROUP), addr -> addr.equals(hp), true) + .getTabletServer(rg -> rg.equals(TEST_RESOURCE_GROUP), AddressPredicate.exact(hp), true) .isEmpty()); EasyMock.verify(ctx, zc); @@ -812,7 +813,7 @@ public void testGetTabletServers() { // query for a specific server results = ctx.getServerPaths().getTabletServer(rg -> rg.equals(TEST_RESOURCE_GROUP), - addr -> addr.equals(hp), true); + AddressPredicate.exact(hp), true); assertEquals(1, results.size()); iter = results.iterator(); slp1 = iter.next(); @@ -823,7 +824,7 @@ public void testGetTabletServers() { // query for a wrong server results = ctx.getServerPaths().getTabletServer(rg -> rg.equals(TEST_RESOURCE_GROUP), - addr -> addr.equals(HostAndPort.fromString("localhost:1234")), true); + AddressPredicate.exact(HostAndPort.fromString("localhost:1234")), true); assertEquals(0, results.size()); EasyMock.verify(ctx, zc); @@ -849,9 +850,8 @@ public void testGetDeadTabletServersNone() { assertTrue(ctx.getServerPaths().getDeadTabletServer(rg -> true, addr -> true, false).isEmpty()); assertTrue(ctx.getServerPaths() .getDeadTabletServer(rg -> rg.equals(TEST_RESOURCE_GROUP), addr -> true, false).isEmpty()); - assertTrue(ctx.getServerPaths() - .getDeadTabletServer(rg -> rg.equals(TEST_RESOURCE_GROUP), addr -> addr.equals(hp), false) - .isEmpty()); + assertTrue(ctx.getServerPaths().getDeadTabletServer(rg -> rg.equals(TEST_RESOURCE_GROUP), + AddressPredicate.exact(hp), false).isEmpty()); EasyMock.verify(ctx, zc); @@ -946,7 +946,7 @@ public void testGetDeadTabletServers() { // query for a specific server results = ctx.getServerPaths().getDeadTabletServer(rg -> rg.equals(TEST_RESOURCE_GROUP), - addr -> addr.equals(hp), false); + AddressPredicate.exact(hp), false); assertEquals(1, results.size()); iter = results.iterator(); slp1 = iter.next(); @@ -958,7 +958,7 @@ public void testGetDeadTabletServers() { // query for a wrong server results = ctx.getServerPaths().getDeadTabletServer(rg -> rg.equals(TEST_RESOURCE_GROUP), - addr -> addr.equals(HostAndPort.fromString("localhost:1234")), false); + AddressPredicate.exact(HostAndPort.fromString("localhost:1234")), false); assertEquals(0, results.size()); EasyMock.verify(ctx, zc); diff --git a/server/base/src/main/java/org/apache/accumulo/server/manager/LiveTServerSet.java b/server/base/src/main/java/org/apache/accumulo/server/manager/LiveTServerSet.java index e5ed27b6c78..302e1b11612 100644 --- a/server/base/src/main/java/org/apache/accumulo/server/manager/LiveTServerSet.java +++ b/server/base/src/main/java/org/apache/accumulo/server/manager/LiveTServerSet.java @@ -30,7 +30,6 @@ import java.util.Optional; import java.util.Set; import java.util.concurrent.TimeUnit; -import java.util.function.Predicate; import org.apache.accumulo.core.Constants; import org.apache.accumulo.core.clientImpl.thrift.ThriftSecurityException; @@ -41,6 +40,7 @@ import org.apache.accumulo.core.lock.ServiceLock; import org.apache.accumulo.core.lock.ServiceLockData; import org.apache.accumulo.core.lock.ServiceLockPaths; +import org.apache.accumulo.core.lock.ServiceLockPaths.AddressPredicate; import org.apache.accumulo.core.lock.ServiceLockPaths.ResourceGroupPredicate; import org.apache.accumulo.core.lock.ServiceLockPaths.ServiceLockPath; import org.apache.accumulo.core.manager.thrift.TabletServerStatus; @@ -465,10 +465,7 @@ public synchronized void remove(TServerInstance server) { ResourceGroupPredicate rgp = rg2 -> rg.equals(rg2); return rgp; }).orElse(rg -> true); - Predicate addrPredicate = address.map(addr -> { - Predicate ap = addr2 -> addr.equals(addr2); - return ap; - }).orElse(addr -> true); + AddressPredicate addrPredicate = address.map(AddressPredicate::exact).orElse(addr -> true); Set paths = context.getServerPaths().getTabletServer(rgPredicate, addrPredicate, false); if (paths.isEmpty() || paths.size() > 1) { diff --git a/server/base/src/main/java/org/apache/accumulo/server/util/Admin.java b/server/base/src/main/java/org/apache/accumulo/server/util/Admin.java index f1a6c492d4e..7290a536699 100644 --- a/server/base/src/main/java/org/apache/accumulo/server/util/Admin.java +++ b/server/base/src/main/java/org/apache/accumulo/server/util/Admin.java @@ -77,6 +77,7 @@ import org.apache.accumulo.core.fate.zookeeper.ZooCache; import org.apache.accumulo.core.fate.zookeeper.ZooReaderWriter; import org.apache.accumulo.core.lock.ServiceLock; +import org.apache.accumulo.core.lock.ServiceLockPaths; import org.apache.accumulo.core.lock.ServiceLockPaths.ServiceLockPath; import org.apache.accumulo.core.manager.thrift.FateService; import org.apache.accumulo.core.manager.thrift.TFateId; @@ -679,8 +680,8 @@ static String getTServersZkPath(ClientContext context) { static String qualifyWithZooKeeperSessionId(ClientContext context, ZooCache zooCache, String hostAndPort) { var hpObj = HostAndPort.fromString(hostAndPort); - Set paths = - context.getServerPaths().getTabletServer(rg -> true, addr -> addr.equals(hpObj), true); + Set paths = context.getServerPaths().getTabletServer(rg -> true, + ServiceLockPaths.AddressPredicate.exact(hpObj), true); if (paths.size() != 1) { return hostAndPort; } diff --git a/server/base/src/main/java/org/apache/accumulo/server/util/TabletServerLocks.java b/server/base/src/main/java/org/apache/accumulo/server/util/TabletServerLocks.java index 32255b1e199..d46ffdb8406 100644 --- a/server/base/src/main/java/org/apache/accumulo/server/util/TabletServerLocks.java +++ b/server/base/src/main/java/org/apache/accumulo/server/util/TabletServerLocks.java @@ -26,6 +26,7 @@ import org.apache.accumulo.core.lock.ServiceLock; import org.apache.accumulo.core.lock.ServiceLockData; import org.apache.accumulo.core.lock.ServiceLockData.ThriftService; +import org.apache.accumulo.core.lock.ServiceLockPaths; import org.apache.accumulo.core.lock.ServiceLockPaths.ServiceLockPath; import org.apache.accumulo.server.ServerContext; @@ -64,7 +65,7 @@ public static void execute(final ServerContext context, final String lock, final } else { var hostAndPort = HostAndPort.fromString(lock); Set paths = context.getServerPaths().getTabletServer(rg -> true, - addr -> addr.equals(hostAndPort), true); + ServiceLockPaths.AddressPredicate.exact(hostAndPort), true); Preconditions.checkArgument(paths.size() == 1, lock + " does not match a single ZooKeeper TabletServer lock. matches=" + paths); ServiceLockPath path = paths.iterator().next();