ctubbsii commented on code in PR #5145:
URL: https://github.com/apache/accumulo/pull/5145#discussion_r1873711942
##########
core/src/main/java/org/apache/accumulo/core/fate/zookeeper/ServiceLock.java:
##########
@@ -762,4 +762,26 @@ public static boolean deleteLock(ZooReaderWriter zk,
ServiceLockPath path, Strin
return false;
}
+
+ /**
+ * Checks that the lock still exists in ZooKeeper. The typical mechanism for
determining if a lock
+ * is lost depends on a Watcher set on the lock node. There exists a case
where the Watcher may
+ * not get called if another Watcher is stuck waiting on I/O or otherwise
hung. In the case where
+ * this method returns false, then the typical action is to exit the server
process.
+ *
+ * @return true if lock path still exists, false otherwise and on error
+ */
+ public boolean verifyLockAtSource() {
+ if (getLockPath() == null) {
+ // lock not set yet
+ return false;
+ }
+ try {
+ return null != this.zooKeeper.exists(getLockPath(), false);
Review Comment:
`this.zooKeeper` is almost certainly coming from a shared ZooKeeper instance
created from ZooSession and is reused by other parts of the system with other
watchers. I'm not sure if that matters here, since this isn't setting a watcher.
This ambiguity over where the ZooKeeper is being created and how it is
reused is something I'm addressing in #5124 and am nearly done. I was not
expecting to make the changes in 2.1, though. I'm only targeting 3.1. I don't
know if it matters for this, but I thought it might make sense to point out, in
case you were expecting this to be a unique ZK client.
##########
server/base/src/main/java/org/apache/accumulo/server/AbstractServer.java:
##########
@@ -139,6 +149,41 @@ public String getApplicationName() {
return applicationName;
}
+ /**
+ * Get the ServiceLock for this server process. May return null if called
before the lock is
+ * acquired.
+ *
+ * @return lock ServiceLock or null
+ */
+ public abstract ServiceLock getLock();
Review Comment:
I think you could avoid making this abstract and needing to implement it in
the child classes if each child class called the super constructor and provided
a `Supplier<ServiceLock>`, then this impl would just be:
```suggestion
public ServiceLock getLock() {
serviceLockSupplier.get();
}
```
That might also avoid issues with handling null here, because the supplier
would likely be blocking.
##########
server/base/src/main/java/org/apache/accumulo/server/AbstractServer.java:
##########
@@ -44,6 +48,8 @@ public abstract class AbstractServer implements
AutoCloseable, MetricsProducer,
private final ProcessMetrics processMetrics;
protected final long idleReportingPeriodNanos;
private volatile long idlePeriodStartNanos = 0L;
+ private volatile Thread serverThread;
+ private volatile Thread verificationThread;
Review Comment:
Do these need to be volatile? I'm not sure what that's getting us here.
Should these be final and the Thread created in the constructor, and only run
in the runServer 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
