You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "ctubbsii (via GitHub)" <gi...@apache.org> on 2023/09/28 08:01:38 UTC
[GitHub] [accumulo] ctubbsii opened a new pull request, #3788: Fix use of ServiceLockData Optional and nulls
ctubbsii opened a new pull request, #3788:
URL: https://github.com/apache/accumulo/pull/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
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #3788: Fix use of ServiceLockData Optional and nulls
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3788:
URL: https://github.com/apache/accumulo/pull/3788#discussion_r1340743591
##########
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:
Before this change, this would have resulted in attempting to make a connection to a server using null for the address. That would have resulted in some exception, though I don't know off the top of my head whether that would be an NPE or an IOException, and caught further down, which would have resulted in a similar message, but reporting that it failed to connect to the GC at the "null" address. This just short-circuits that attempt to connect using a null address and fail later with a non-sensical message. It's not strictly necessary, but I thought it'd be a better behavior.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #3788: Fix use of ServiceLockData Optional and nulls
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3788:
URL: https://github.com/apache/accumulo/pull/3788#discussion_r1340747373
##########
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:
It could be possible that lock data was serialized to that node, but did not have the TSERV service registered (that would probably be a bigger bug if that were to happen). If that were to happen before my fix, this method would have blown up with an NPE in a similar way to the original reported issue. Now, the address would be null, and it simply won't get added to the list of detected servers... which was the original intent with the null check, as demonstrated by the way it behaved in previous versions of Accumulo (I consulted 2.1 behavior when making these changes).
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a diff in pull request #3788: Fix use of ServiceLockData Optional and nulls
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3788:
URL: https://github.com/apache/accumulo/pull/3788#discussion_r1340742061
##########
core/src/main/java/org/apache/accumulo/core/lock/ServiceLockData.java:
##########
@@ -164,30 +165,22 @@ public ServiceLockData(UUID uuid, String address, ThriftService service) {
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);
Review Comment:
Leveraging the existing optional and replacing if stmts with Optional.map stmts was a really nice change.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #3788: Fix use of ServiceLockData Optional and nulls
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3788:
URL: https://github.com/apache/accumulo/pull/3788#discussion_r1340738031
##########
core/src/main/java/org/apache/accumulo/core/lock/ServiceLockData.java:
##########
@@ -164,30 +165,22 @@ public ServiceLockData(UUID uuid, String address, ThriftService service) {
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);
Review Comment:
This is the main change that fixes things in this PR. The other changes in this class were just taking the opportunity to make the other methods that do the same thing a bit more concise and consistent with one another.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #3788: Fix use of ServiceLockData Optional and nulls
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3788:
URL: https://github.com/apache/accumulo/pull/3788#discussion_r1340744766
##########
core/src/main/java/org/apache/accumulo/core/lock/ServiceLockData.java:
##########
@@ -164,30 +165,22 @@ public ServiceLockData(UUID uuid, String address, ThriftService service) {
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);
Review Comment:
It wasn't strictly necessary to do that to fix the issue, but it made the code so much easier to understand while I was trying to track down and verify there were no negative side-effects by any of the existing callers when I changed the behavior of this getAddress method.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a diff in pull request #3788: Fix use of ServiceLockData Optional and nulls
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii merged pull request #3788: Fix use of ServiceLockData Optional and nulls
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii merged PR #3788:
URL: https://github.com/apache/accumulo/pull/3788
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org