keith-turner commented on code in PR #3788:
URL: https://github.com/apache/accumulo/pull/3788#discussion_r1340734093
##########
core/src/main/java/org/apache/accumulo/core/rpc/clients/TServerClient.java:
##########
@@ -68,13 +64,9 @@ default Pair<String,C> getTabletServerConnection(Logger LOG,
ThriftClientTypes<C
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))
Review Comment:
The old code was checking to see if getAddress() returned null, however
looking at the code it does not seem like it ever would return null. However
if it does it seems that Optional.map handles this nicely.
##########
server/monitor/src/main/java/org/apache/accumulo/monitor/Monitor.java:
##########
@@ -428,10 +428,11 @@ private GCStatus fetchGcStatus() {
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;
Review Comment:
The rest of the code is ok with this returning null?
Before these change could this code have thrown an NPE?
Not sure about the log level of warn for the new log message. Sometimes the
GC is down and that is expected and the warn would not be useful in that case.
When the GC is down and its not expected, these log messages are probably not a
great way to do detect that. I would go with debug for this log message.
--
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]
