This is an automated email from the ASF dual-hosted git repository.
ctubbsii 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 b9fdbc3beb Fix use of ServiceLockData Optional and nulls (#3788)
b9fdbc3beb is described below
commit b9fdbc3beb4bb7e3c6c7e132ad91f4bff1894259
Author: Christopher Tubbs <ctubb...@apache.org>
AuthorDate: Thu Sep 28 19:27:14 2023 -0400
Fix use of ServiceLockData Optional and nulls (#3788)
* Simplify ServiceLockData's if conditions in methods that return null
* Ensure ServiceLockData.getAddress() returns null when
.getAddressString() returns null
* Handle null toString() in ServiceLockData.equals() using
Objects.equals()
* Simplify TabletMetadata.checkServer(),
TServerClient.getTabletServerConnection(),
ExternalCompactionUtil.findCompactionCoordinator(),
LiveTServerSet.checkServer(), and Monitor.fetchGcStatus() by using
Optional.map(), Optional.ifPresent(), and Optional.orElse(null)
* Improve Monitor.fetchGcStatus() by short-circuiting the warning and
returning null results when the address is null, rather than wait for
an exception to be thrown while trying to connect to the service using
a null address
* Update ServiceLockDataTest to ensure getAddress() returns null when
it's expected to, instead of throwing a NullPointerException
This fixes #3782
---
.../apache/accumulo/core/lock/ServiceLockData.java | 30 +++++++---------------
.../core/metadata/schema/TabletMetadata.java | 15 +++--------
.../accumulo/core/rpc/clients/TServerClient.java | 14 +++-------
.../util/compaction/ExternalCompactionUtil.java | 8 ++----
.../accumulo/core/lock/ServiceLockDataTest.java | 9 ++++---
.../accumulo/server/manager/LiveTServerSet.java | 13 +++++-----
.../java/org/apache/accumulo/monitor/Monitor.java | 9 ++++---
7 files changed, 35 insertions(+), 63 deletions(-)
diff --git
a/core/src/main/java/org/apache/accumulo/core/lock/ServiceLockData.java
b/core/src/main/java/org/apache/accumulo/core/lock/ServiceLockData.java
index 736a36f0fe..865c0a9f4c 100644
--- a/core/src/main/java/org/apache/accumulo/core/lock/ServiceLockData.java
+++ b/core/src/main/java/org/apache/accumulo/core/lock/ServiceLockData.java
@@ -25,6 +25,7 @@ import static
org.apache.accumulo.core.util.LazySingletons.GSON;
import java.util.Collections;
import java.util.EnumMap;
import java.util.HashSet;
+import java.util.Objects;
import java.util.Optional;
import java.util.Set;
import java.util.UUID;
@@ -164,30 +165,22 @@ public class ServiceLockData implements
Comparable<ServiceLockData> {
public String getAddressString(ThriftService service) {
ServiceDescriptor sd = services.get(service);
- if (sd == null) {
- return null;
- }
- return sd.getAddress();
+ return sd == null ? null : sd.getAddress();
}
public HostAndPort getAddress(ThriftService service) {
- return AddressUtil.parseAddress(getAddressString(service), false);
+ String s = getAddressString(service);
+ return s == null ? null : AddressUtil.parseAddress(s, false);
}
public String getGroup(ThriftService service) {
ServiceDescriptor sd = services.get(service);
- if (sd == null) {
- return null;
- }
- return sd.getGroup();
+ return sd == null ? null : sd.getGroup();
}
public UUID getServerUUID(ThriftService service) {
ServiceDescriptor sd = services.get(service);
- if (sd == null) {
- return null;
- }
- return sd.getUUID();
+ return sd == null ? null : sd.getUUID();
}
public byte[] serialize() {
@@ -208,10 +201,7 @@ public class ServiceLockData implements
Comparable<ServiceLockData> {
@Override
public boolean equals(Object o) {
- if (o instanceof ServiceLockData) {
- return toString().equals(o.toString());
- }
- return false;
+ return o instanceof ServiceLockData ? Objects.equals(toString(),
o.toString()) : false;
}
@Override
@@ -224,10 +214,8 @@ public class ServiceLockData implements
Comparable<ServiceLockData> {
return Optional.empty();
}
String data = new String(lockData, UTF_8);
- if (data.isBlank()) {
- return Optional.empty();
- }
- return Optional.of(new ServiceLockData(GSON.get().fromJson(data,
ServiceDescriptors.class)));
+ return data.isBlank() ? Optional.empty()
+ : Optional.of(new ServiceLockData(GSON.get().fromJson(data,
ServiceDescriptors.class)));
}
}
diff --git
a/core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletMetadata.java
b/core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletMetadata.java
index bc58b2b806..fccc3e6c43 100644
---
a/core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletMetadata.java
+++
b/core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletMetadata.java
@@ -538,18 +538,11 @@ public class TabletMetadata {
*/
private static Optional<TServerInstance> checkServer(ClientContext context,
String path,
String zPath) {
- Optional<TServerInstance> server = Optional.empty();
final var lockPath = ServiceLock.path(path + "/" + zPath);
ZooCache.ZcStat stat = new ZooCache.ZcStat();
- Optional<ServiceLockData> sld =
ServiceLock.getLockData(context.getZooCache(), lockPath, stat);
-
- if (sld.isPresent()) {
- log.trace("Checking server at ZK path = " + lockPath);
- HostAndPort client =
sld.orElseThrow().getAddress(ServiceLockData.ThriftService.TSERV);
- if (client != null) {
- server = Optional.of(new TServerInstance(client,
stat.getEphemeralOwner()));
- }
- }
- return server;
+ log.trace("Checking server at ZK path = " + lockPath);
+ return ServiceLock.getLockData(context.getZooCache(), lockPath, stat)
+ .map(sld -> sld.getAddress(ServiceLockData.ThriftService.TSERV))
+ .map(address -> new TServerInstance(address,
stat.getEphemeralOwner()));
}
}
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 e0621dacc8..c60c91c5d6 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
@@ -23,7 +23,6 @@ import static
com.google.common.util.concurrent.Uninterruptibles.sleepUninterrup
import static java.util.concurrent.TimeUnit.MILLISECONDS;
import java.util.ArrayList;
-import java.util.Optional;
import java.util.concurrent.atomic.AtomicBoolean;
import org.apache.accumulo.core.Constants;
@@ -35,7 +34,6 @@ import org.apache.accumulo.core.clientImpl.ThriftTransportKey;
import org.apache.accumulo.core.clientImpl.thrift.ThriftSecurityException;
import org.apache.accumulo.core.fate.zookeeper.ZooCache;
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.rpc.ThriftUtil;
import org.apache.accumulo.core.rpc.clients.ThriftClientTypes.Exec;
@@ -48,8 +46,6 @@ import org.apache.thrift.transport.TTransport;
import org.apache.thrift.transport.TTransportException;
import org.slf4j.Logger;
-import com.google.common.net.HostAndPort;
-
public interface TServerClient<C extends TServiceClient> {
Pair<String,C> getTabletServerConnection(ClientContext context, boolean
preferCachedConnections)
@@ -68,13 +64,9 @@ public interface TServerClient<C extends TServiceClient> {
for (String tserver : zc.getChildren(context.getZooKeeperRoot() +
Constants.ZTSERVERS)) {
var zLocPath =
ServiceLock.path(context.getZooKeeperRoot() + Constants.ZTSERVERS +
"/" + tserver);
- Optional<ServiceLockData> sld = zc.getLockData(zLocPath);
- if (sld.isPresent()) {
- HostAndPort address =
sld.orElseThrow().getAddress(ThriftService.TSERV);
- if (address != null) {
- servers.add(new ThriftTransportKey(address, rpcTimeout, context));
- }
- }
+ zc.getLockData(zLocPath).map(sld -> sld.getAddress(ThriftService.TSERV))
+ .map(address -> new ThriftTransportKey(address, rpcTimeout, context))
+ .ifPresent(servers::add);
}
boolean opened = false;
diff --git
a/core/src/main/java/org/apache/accumulo/core/util/compaction/ExternalCompactionUtil.java
b/core/src/main/java/org/apache/accumulo/core/util/compaction/ExternalCompactionUtil.java
index 0bc811dfbd..4e40165f09 100644
---
a/core/src/main/java/org/apache/accumulo/core/util/compaction/ExternalCompactionUtil.java
+++
b/core/src/main/java/org/apache/accumulo/core/util/compaction/ExternalCompactionUtil.java
@@ -36,7 +36,6 @@ import
org.apache.accumulo.core.compaction.thrift.CompactorService;
import org.apache.accumulo.core.fate.zookeeper.ZooReader;
import org.apache.accumulo.core.fate.zookeeper.ZooSession;
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.metadata.schema.ExternalCompactionId;
import org.apache.accumulo.core.rpc.ThriftUtil;
@@ -104,11 +103,8 @@ public class ExternalCompactionUtil {
try {
var zk = ZooSession.getAnonymousSession(context.getZooKeepers(),
context.getZooKeepersSessionTimeOut());
- Optional<ServiceLockData> sld = ServiceLock.getLockData(zk,
ServiceLock.path(lockPath));
- if (sld.isEmpty()) {
- return Optional.empty();
- }
- return
Optional.ofNullable(sld.orElseThrow().getAddress(ThriftService.COORDINATOR));
+ return ServiceLock.getLockData(zk, ServiceLock.path(lockPath))
+ .map(sld -> sld.getAddress(ThriftService.COORDINATOR));
} catch (KeeperException | InterruptedException e) {
throw new IllegalStateException(e);
}
diff --git
a/core/src/test/java/org/apache/accumulo/core/lock/ServiceLockDataTest.java
b/core/src/test/java/org/apache/accumulo/core/lock/ServiceLockDataTest.java
index 5f89af57b1..1abe862d50 100644
--- a/core/src/test/java/org/apache/accumulo/core/lock/ServiceLockDataTest.java
+++ b/core/src/test/java/org/apache/accumulo/core/lock/ServiceLockDataTest.java
@@ -47,7 +47,7 @@ public class ServiceLockDataTest {
assertEquals(ServiceDescriptor.DEFAULT_GROUP_NAME,
ss.getGroup(ThriftService.TSERV));
assertNull(ss.getServerUUID(ThriftService.TABLET_SCAN));
assertNull(ss.getAddressString(ThriftService.TABLET_SCAN));
- assertThrows(NullPointerException.class, () ->
ss.getAddress(ThriftService.TABLET_SCAN));
+ assertNull(ss.getAddress(ThriftService.TABLET_SCAN));
assertNull(ss.getGroup(ThriftService.TABLET_SCAN));
}
@@ -77,7 +77,7 @@ public class ServiceLockDataTest {
assertEquals("meta", ss.getGroup(ThriftService.TSERV));
assertNull(ss.getServerUUID(ThriftService.TABLET_SCAN));
assertNull(ss.getAddressString(ThriftService.TABLET_SCAN));
- assertThrows(NullPointerException.class, () ->
ss.getAddress(ThriftService.TABLET_SCAN));
+ assertNull(ss.getAddress(ThriftService.TABLET_SCAN));
assertNull(ss.getGroup(ThriftService.TABLET_SCAN));
}
@@ -90,7 +90,7 @@ public class ServiceLockDataTest {
assertEquals("meta", ss.getGroup(ThriftService.TSERV));
assertEquals(serverUUID, ss.getServerUUID(ThriftService.TSERV));
assertNull(ss.getAddressString(ThriftService.TABLET_SCAN));
- assertThrows(NullPointerException.class, () ->
ss.getAddress(ThriftService.TABLET_SCAN));
+ assertNull(ss.getAddress(ThriftService.TABLET_SCAN));
assertNull(ss.getGroup(ThriftService.TABLET_SCAN));
}
@@ -111,6 +111,9 @@ public class ServiceLockDataTest {
assertEquals(HostAndPort.fromString("127.0.0.1:9998"),
ss.getAddress(ThriftService.TABLET_SCAN));
assertEquals("ns1", ss.getGroup(ThriftService.TABLET_SCAN));
+ assertNull(ss.getAddressString(ThriftService.COMPACTOR));
+ assertNull(ss.getAddress(ThriftService.COMPACTOR));
+ assertNull(ss.getGroup(ThriftService.COMPACTOR));
}
@Test
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 04dfcef9e4..e932d5c903 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
@@ -26,7 +26,6 @@ import java.util.HashMap;
import java.util.HashSet;
import java.util.Map;
import java.util.Map.Entry;
-import java.util.Optional;
import java.util.Set;
import java.util.concurrent.TimeUnit;
@@ -297,9 +296,10 @@ public class LiveTServerSet implements Watcher {
final var zLockPath = ServiceLock.path(path + "/" + zPath);
ZcStat stat = new ZcStat();
- Optional<ServiceLockData> sld = ServiceLock.getLockData(getZooCache(),
zLockPath, stat);
+ HostAndPort address = ServiceLock.getLockData(getZooCache(), zLockPath,
stat)
+ .map(sld ->
sld.getAddress(ServiceLockData.ThriftService.TSERV)).orElse(null);
- if (sld.isEmpty()) {
+ if (address == null) {
if (info != null) {
doomed.add(info.instance);
current.remove(zPath);
@@ -315,18 +315,17 @@ public class LiveTServerSet implements Watcher {
}
} else {
locklessServers.remove(zPath);
- HostAndPort client =
sld.orElseThrow().getAddress(ServiceLockData.ThriftService.TSERV);
- TServerInstance instance = new TServerInstance(client,
stat.getEphemeralOwner());
+ TServerInstance instance = new TServerInstance(address,
stat.getEphemeralOwner());
if (info == null) {
updates.add(instance);
- TServerInfo tServerInfo = new TServerInfo(instance, new
TServerConnection(client));
+ TServerInfo tServerInfo = new TServerInfo(instance, new
TServerConnection(address));
current.put(zPath, tServerInfo);
currentInstances.put(instance, tServerInfo);
} else if (!info.instance.equals(instance)) {
doomed.add(info.instance);
updates.add(instance);
- TServerInfo tServerInfo = new TServerInfo(instance, new
TServerConnection(client));
+ TServerInfo tServerInfo = new TServerInfo(instance, new
TServerConnection(address));
current.put(zPath, tServerInfo);
currentInstances.remove(info.instance);
currentInstances.put(instance, tServerInfo);
diff --git
a/server/monitor/src/main/java/org/apache/accumulo/monitor/Monitor.java
b/server/monitor/src/main/java/org/apache/accumulo/monitor/Monitor.java
index 7e1242def6..694d6defb4 100644
--- a/server/monitor/src/main/java/org/apache/accumulo/monitor/Monitor.java
+++ b/server/monitor/src/main/java/org/apache/accumulo/monitor/Monitor.java
@@ -428,10 +428,11 @@ public class Monitor extends AbstractServer implements
HighlyAvailableService {
var path = ServiceLock.path(context.getZooKeeperRoot() +
Constants.ZGC_LOCK);
List<String> locks = ServiceLock.validateAndSort(path,
zk.getChildren(path.toString()));
if (locks != null && !locks.isEmpty()) {
- Optional<ServiceLockData> sld =
- ServiceLockData.parse(zk.getData(path + "/" + locks.get(0)));
- if (sld.isPresent()) {
- address = sld.orElseThrow().getAddress(ThriftService.GC);
+ address = ServiceLockData.parse(zk.getData(path + "/" + locks.get(0)))
+ .map(sld -> sld.getAddress(ThriftService.GC)).orElse(null);
+ if (address == null) {
+ log.warn("Unable to contact the garbage collector (no address)");
+ return null;
}
GCMonitorService.Client client =
ThriftUtil.getClient(ThriftClientTypes.GC, address, context);
