This is an automated email from the ASF dual-hosted git repository.
kturner pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/accumulo.git
The following commit(s) were added to refs/heads/main by this push:
new 2c54f9f2c9 avoids uneeded object allocation on ServiceLocksPath (#4944)
2c54f9f2c9 is described below
commit 2c54f9f2c9c98d0f83905aa08dd98de61d7cd946
Author: Keith Turner <[email protected]>
AuthorDate: Fri Oct 4 13:27:08 2024 -0400
avoids uneeded object allocation on ServiceLocksPath (#4944)
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 +++++++++++-----
.../accumulo/core/rpc/clients/TServerClient.java | 9 +++----
.../accumulo/core/lock/ServiceLockPathsTest.java | 28 +++++++++++-----------
.../accumulo/server/manager/LiveTServerSet.java | 7 ++----
.../org/apache/accumulo/server/util/Admin.java | 5 ++--
.../accumulo/server/util/TabletServerLocks.java | 3 ++-
7 files changed, 45 insertions(+), 35 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 72e4e15d77..2f0367c102 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 java.util.Set;
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 class ZookeeperLockChecker implements
TabletServerLockChecker {
// ServiceLockPaths only returns items that have a lock
var hostAndPort = HostAndPort.fromString(server);
Set<ServiceLockPath> 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 class ZookeeperLockChecker implements
TabletServerLockChecker {
// ServiceLockPaths only returns items that have a lock
var hostAndPort = HostAndPort.fromString(server);
Set<ServiceLockPath> 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 class ZookeeperLockChecker implements
TabletServerLockChecker {
@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 30c94e9ab4..b60712e92b 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 class ServiceLockPaths {
}
public Set<ServiceLockPath> getCompactor(ResourceGroupPredicate
resourceGroupPredicate,
- Predicate<HostAndPort> address, boolean withLock) {
+ AddressPredicate address, boolean withLock) {
return get(Constants.ZCOMPACTORS, resourceGroupPredicate, address,
withLock);
}
@@ -316,17 +316,17 @@ public class ServiceLockPaths {
}
public Set<ServiceLockPath> getScanServer(ResourceGroupPredicate
resourceGroupPredicate,
- Predicate<HostAndPort> address, boolean withLock) {
+ AddressPredicate address, boolean withLock) {
return get(Constants.ZSSERVERS, resourceGroupPredicate, address, withLock);
}
public Set<ServiceLockPath> getTabletServer(ResourceGroupPredicate
resourceGroupPredicate,
- Predicate<HostAndPort> address, boolean withLock) {
+ AddressPredicate address, boolean withLock) {
return get(Constants.ZTSERVERS, resourceGroupPredicate, address, withLock);
}
public Set<ServiceLockPath> getDeadTabletServer(ResourceGroupPredicate
resourceGroupPredicate,
- Predicate<HostAndPort> address, boolean withLock) {
+ AddressPredicate address, boolean withLock) {
return get(Constants.ZDEADTSERVERS, resourceGroupPredicate, address,
withLock);
}
@@ -334,6 +334,15 @@ public class ServiceLockPaths {
}
+ public interface AddressPredicate extends Predicate<String> {
+
+ 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 class ServiceLockPaths {
* @return set of ServiceLockPath objects for the paths found based on the
search criteria
*/
private Set<ServiceLockPath> get(final String serverType,
- ResourceGroupPredicate resourceGroupPredicate, Predicate<HostAndPort>
addressPredicate,
+ ResourceGroupPredicate resourceGroupPredicate, AddressPredicate
addressPredicate,
boolean withLock) {
Objects.requireNonNull(serverType);
@@ -380,7 +389,7 @@ public class ServiceLockPaths {
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/main/java/org/apache/accumulo/core/rpc/clients/TServerClient.java
b/core/src/main/java/org/apache/accumulo/core/rpc/clients/TServerClient.java
index e875536ff2..213e8ffec3 100644
--- a/core/src/main/java/org/apache/accumulo/core/rpc/clients/TServerClient.java
+++ b/core/src/main/java/org/apache/accumulo/core/rpc/clients/TServerClient.java
@@ -39,6 +39,7 @@ import
org.apache.accumulo.core.clientImpl.thrift.ThriftSecurityException;
import org.apache.accumulo.core.fate.zookeeper.ZooCache;
import org.apache.accumulo.core.lock.ServiceLockData;
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.apache.accumulo.core.rpc.ThriftUtil;
import org.apache.accumulo.core.rpc.clients.ThriftClientTypes.Exec;
@@ -86,12 +87,12 @@ public interface TServerClient<C extends TServiceClient> {
// that the path is correct and the lock is held and will return the
// correct one.
HostAndPort hp = HostAndPort.fromString(debugHost);
- serverPaths
- .addAll(context.getServerPaths().getCompactor(rg -> true, addr ->
addr.equals(hp), true));
serverPaths.addAll(
- context.getServerPaths().getScanServer(rg -> true, addr ->
addr.equals(hp), true));
+ context.getServerPaths().getCompactor(rg -> true,
AddressPredicate.exact(hp), true));
serverPaths.addAll(
- context.getServerPaths().getTabletServer(rg -> true, addr ->
addr.equals(hp), true));
+ context.getServerPaths().getScanServer(rg -> true,
AddressPredicate.exact(hp), true));
+ serverPaths.addAll(
+ context.getServerPaths().getTabletServer(rg -> true,
AddressPredicate.exact(hp), true));
} else {
serverPaths.addAll(context.getServerPaths().getTabletServer(rg -> true,
addr -> true, true));
if (type == ThriftClientTypes.CLIENT) {
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 3f91c756b7..27e1e795a2 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.clientImpl.ClientContext;
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 class ServiceLockPathsTest {
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 class ServiceLockPathsTest {
// 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 class ServiceLockPathsTest {
// 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 class ServiceLockPathsTest {
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 class ServiceLockPathsTest {
// 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 class ServiceLockPathsTest {
// 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 class ServiceLockPathsTest {
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 class ServiceLockPathsTest {
// 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 class ServiceLockPathsTest {
// 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 class ServiceLockPathsTest {
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 class ServiceLockPathsTest {
// 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 class ServiceLockPathsTest {
// 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 e5ed27b6c7..302e1b1161 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.Map.Entry;
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.fate.zookeeper.ZooCache.ZcStat;
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 class LiveTServerSet implements Watcher {
ResourceGroupPredicate rgp = rg2 -> rg.equals(rg2);
return rgp;
}).orElse(rg -> true);
- Predicate<HostAndPort> addrPredicate = address.map(addr -> {
- Predicate<HostAndPort> ap = addr2 -> addr.equals(addr2);
- return ap;
- }).orElse(addr -> true);
+ AddressPredicate addrPredicate =
address.map(AddressPredicate::exact).orElse(addr -> true);
Set<ServiceLockPath> 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 f1a6c492d4..7290a53669 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.MetaFateStore;
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 @@ public class Admin implements KeywordExecutable {
static String qualifyWithZooKeeperSessionId(ClientContext context, ZooCache
zooCache,
String hostAndPort) {
var hpObj = HostAndPort.fromString(hostAndPort);
- Set<ServiceLockPath> paths =
- context.getServerPaths().getTabletServer(rg -> true, addr ->
addr.equals(hpObj), true);
+ Set<ServiceLockPath> 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 32255b1e19..d46ffdb840 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.fate.zookeeper.ZooReaderWriter;
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 class TabletServerLocks {
} else {
var hostAndPort = HostAndPort.fromString(lock);
Set<ServiceLockPath> 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();
