ctubbsii commented on code in PR #5246:
URL: https://github.com/apache/accumulo/pull/5246#discussion_r1912344290
##########
minicluster/src/main/java/org/apache/accumulo/miniclusterImpl/MiniAccumuloClusterControl.java:
##########
@@ -278,6 +279,15 @@ public synchronized void stop(ServerType server, String
hostname) throws IOExcep
if (managerProcess != null) {
try {
cluster.stopProcessWithTimeout(managerProcess, 30,
TimeUnit.SECONDS);
+ try {
+ System.setProperty("accumulo.properties",
+ "file://" + cluster.getAccumuloPropertiesPath());
+ new
ZooZap().zap(cluster.getServerContext().getSiteConfiguration(),
+ new String[] {"-manager"});
+ } catch (Exception e) {
+ log.error("Error zapping Manager zookeeper lock", e);
+ }
Review Comment:
Setting a system property like this seems dubious. This is something a user
could have set in their environment, and it's not clear the behavior if we rely
on it. We should be able to call the ZooZap utility's API with the config we
want to use directly, rather than relying on setting something in the
environment for it to use.
##########
minicluster/src/main/java/org/apache/accumulo/miniclusterImpl/MiniAccumuloClusterImpl.java:
##########
@@ -824,9 +825,47 @@ public synchronized void stop() throws IOException,
InterruptedException {
control.stop(ServerType.GARBAGE_COLLECTOR, null);
control.stop(ServerType.MANAGER, null);
+ control.stop(ServerType.COMPACTION_COORDINATOR);
control.stop(ServerType.TABLET_SERVER, null);
+ control.stop(ServerType.COMPACTOR, null);
+ control.stop(ServerType.SCAN_SERVER, null);
+
+ // The method calls above kill the server
+ // Clean up the locks in ZooKeeper fo that if the cluster
+ // is restarted, then the processes will start right away
+ // and not wait for the old locks to be cleaned up.
+ try {
+ System.setProperty("accumulo.properties", "file://" +
getAccumuloPropertiesPath());
+ new ZooZap().zap(getServerContext().getSiteConfiguration(), new String[]
{"-manager",
+ "-compaction-coordinators", "-tservers", "-compactors",
"-sservers"});
+ } catch (Exception e) {
+ log.error("Error zapping zookeeper locks", e);
+ }
control.stop(ServerType.ZOOKEEPER, null);
+ // Clear the location of the servers in ZooCache.
+ // When ZooKeeper was stopped in the previous method call,
+ // the local ZooKeeper watcher did not fire. If MAC is
+ // restarted, then ZooKeeper will start on the same port with
+ // the same data, but no Watchers will fire.
Review Comment:
Does MAC even have the ability to restart? I don't think that's a feature we
have, is it? Regardless, I think watchers would fire with SessionExpired in
this case, which the ZooCache watcher does handle correctly. I believe my
ZooSession from #5192, which was just merged to the main branch should also
handle this correctly.
##########
core/src/main/java/org/apache/accumulo/core/fate/zookeeper/ZooCache.java:
##########
@@ -525,6 +526,36 @@ public void clear(String zPath) {
}
}
+ /**
+ * Removes all paths in the cache match the predicate.
+ */
+ public void clear(Predicate<String> pathPredicate) {
+ Preconditions.checkState(!closed);
+ Predicate<String> pathPredicateToUse;
+ if (log.isTraceEnabled()) {
+ pathPredicateToUse = path -> {
+ boolean testResult = pathPredicate.test(path);
+ if (testResult) {
+ log.trace("removing {} from cache", path);
+ }
+ return testResult;
+ };
Review Comment:
Would this work?
```suggestion
pathPredicateToUse = pathPredicate.and(path -> {
log.trace("removing {} from cache", path);
return true;
});
```
##########
minicluster/src/main/java/org/apache/accumulo/miniclusterImpl/MiniAccumuloClusterImpl.java:
##########
@@ -824,9 +825,47 @@ public synchronized void stop() throws IOException,
InterruptedException {
control.stop(ServerType.GARBAGE_COLLECTOR, null);
control.stop(ServerType.MANAGER, null);
+ control.stop(ServerType.COMPACTION_COORDINATOR);
control.stop(ServerType.TABLET_SERVER, null);
+ control.stop(ServerType.COMPACTOR, null);
+ control.stop(ServerType.SCAN_SERVER, null);
+
+ // The method calls above kill the server
+ // Clean up the locks in ZooKeeper fo that if the cluster
+ // is restarted, then the processes will start right away
+ // and not wait for the old locks to be cleaned up.
+ try {
+ System.setProperty("accumulo.properties", "file://" +
getAccumuloPropertiesPath());
Review Comment:
Same comment about the system property. Prefer controlling ZooZap directly
through its API.
--
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]
