You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "dlmarion (via GitHub)" <gi...@apache.org> on 2025/01/10 22:53:28 UTC
[PR] Made ITs that restart MAC faster [accumulo]
dlmarion opened a new pull request, #5246:
URL: https://github.com/apache/accumulo/pull/5246
Modified MAC so that it cleaned up lock paths
in ZooKeeper and ZooCache when stopping. Noticed
that in tests that restarted MAC the Manager
process would wait for the previous lock to be
removed on the session timeout. The lock paths
would also be cached in ZooCache and not updated
right away because the Watcher would not fire
when MAC was stopped, a ConnectionLoss error
would be returned when MAC started, and it would
take a while for ZooCache to fix itself.
--
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
Re: [PR] Made ITs that restart MAC faster [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #5246:
URL: https://github.com/apache/accumulo/pull/5246#discussion_r1913231183
##########
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:
Yes, I think this would work. Based on the javadoc it looks like if `pathPredicate` returns false, then the logging will not occur.
--
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
Re: [PR] Made ITs that restart MAC faster [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #5246:
URL: https://github.com/apache/accumulo/pull/5246#discussion_r1914221231
##########
minicluster/src/main/java/org/apache/accumulo/miniclusterImpl/MiniAccumuloClusterImpl.java:
##########
@@ -824,9 +825,46 @@ 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 {
+ new ZooZap().zap(getServerContext().getSiteConfiguration(), new String[] {"-manager",
+ "-compaction-coordinators", "-tservers", "-compactors", "-sservers"});
+ } catch (Exception e) {
+ log.error("Error zapping zookeeper locks", e);
+ }
Review Comment:
Should catch exceptions more narrowly. The stop() method in which these exist is already throwing some that we don't need to bother catching.
##########
minicluster/src/main/java/org/apache/accumulo/miniclusterImpl/MiniAccumuloClusterControl.java:
##########
@@ -278,6 +279,13 @@ public synchronized void stop(ServerType server, String hostname) throws IOExcep
if (managerProcess != null) {
try {
cluster.stopProcessWithTimeout(managerProcess, 30, TimeUnit.SECONDS);
+ try {
+ new ZooZap().zap(cluster.getServerContext().getSiteConfiguration(),
+ new String[] {"-manager"});
Review Comment:
This would probably look nicer if ZooZap took varargs instead of a `String[]`
Should this go in the finally block instead of in the same block that's handling the 30 second timeout?
##########
minicluster/src/main/java/org/apache/accumulo/miniclusterImpl/MiniAccumuloClusterControl.java:
##########
@@ -278,6 +279,13 @@ public synchronized void stop(ServerType server, String hostname) throws IOExcep
if (managerProcess != null) {
try {
cluster.stopProcessWithTimeout(managerProcess, 30, TimeUnit.SECONDS);
+ try {
+ new ZooZap().zap(cluster.getServerContext().getSiteConfiguration(),
+ new String[] {"-manager"});
+ } catch (Exception e) {
+ log.error("Error zapping Manager zookeeper lock", e);
+ }
Review Comment:
This should catch exceptions more narrowly
##########
server/base/src/main/java/org/apache/accumulo/server/util/ZooZap.java:
##########
@@ -90,6 +90,19 @@ public static void main(String[] args) throws Exception {
@Override
public void execute(String[] args) throws Exception {
+ try {
+ var siteConf = SiteConfiguration.auto();
+ // Login as the server on secure HDFS
+ if (siteConf.getBoolean(Property.INSTANCE_RPC_SASL_ENABLED)) {
+ SecurityUtil.serverLogin(siteConf);
+ }
+ zap(siteConf, args);
+ } finally {
+ SingletonManager.setMode(Mode.CLOSED);
+ }
+ }
+
+ public void zap(SiteConfiguration siteConf, String[] args) {
Review Comment:
```suggestion
public void zap(SiteConfiguration siteConf, String ... args) {
```
--
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
Re: [PR] Made ITs that restart MAC faster [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #5246:
URL: https://github.com/apache/accumulo/pull/5246#discussion_r1912345525
##########
server/base/src/main/java/org/apache/accumulo/server/util/ZooZap.java:
##########
@@ -90,6 +90,19 @@ public static void main(String[] args) throws Exception {
@Override
public void execute(String[] args) throws Exception {
+ try {
+ var siteConf = SiteConfiguration.auto();
+ // Login as the server on secure HDFS
+ if (siteConf.getBoolean(Property.INSTANCE_RPC_SASL_ENABLED)) {
+ SecurityUtil.serverLogin(siteConf);
+ }
+ zap(siteConf, args);
Review Comment:
Looking at the diff ignoring whitespace changes makes this easier to see. It looks like the only thing that changed was that the jcommander option parsing got moved into the try block. Otherwise, I don't think there's any substantive change in this part.
--
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
Re: [PR] Made ITs that restart MAC faster [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #5246:
URL: https://github.com/apache/accumulo/pull/5246#discussion_r1913278011
##########
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:
Removed system property in dc5e2c3. It wasn't needed anymore, it was needed when I was calling `ZooZap.execute` directly before I added `ZooZap.zap`
--
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
Re: [PR] Made ITs that restart MAC faster [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #5246:
URL: https://github.com/apache/accumulo/pull/5246#discussion_r1913276372
##########
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:
Added this change in dc5e2c3
--
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
Re: [PR] Made ITs that restart MAC faster [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #5246:
URL: https://github.com/apache/accumulo/pull/5246#issuecomment-2584661101
These changes shaved about 30s off of VolumeIT locally for me in 2.1. In main, with more tests, it shaved off about 50s.
--
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
Re: [PR] Made ITs that restart MAC faster [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #5246:
URL: https://github.com/apache/accumulo/pull/5246#discussion_r1914216953
##########
minicluster/src/main/java/org/apache/accumulo/miniclusterImpl/MiniAccumuloClusterImpl.java:
##########
@@ -824,9 +825,46 @@ 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 {
+ 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.
+ boolean startCalled = true;
+ try {
+ getServerContext();
+ } catch (RuntimeException e) {
+ if (e.getMessage().startsWith("Accumulo not initialized")) {
+ startCalled = false;
+ }
+ }
+ if (startCalled) {
+ final ServerContext ctx = getServerContext();
+ final String zRoot = getServerContext().getZooKeeperRoot();
+ ctx.getZooCache().clear((path) -> path.startsWith(zRoot + Constants.ZMANAGER_LOCK));
+ ctx.getZooCache().clear((path) -> path.startsWith(zRoot + Constants.ZGC_LOCK));
+ ctx.getZooCache().clear((path) -> path.startsWith(zRoot + Constants.ZCOMPACTORS));
+ ctx.getZooCache().clear((path) -> path.startsWith(zRoot + Constants.ZSSERVERS));
+ ctx.getZooCache().clear((path) -> path.startsWith(zRoot + Constants.ZTSERVERS));
Review Comment:
I don't know if this is helpful, but you can define the predicate once outside this code, and then just call clear a single time with a single ready-to-go predicate.
```suggestion
Predicate<String> lockPaths = path -> path.startsWith(zRoot + Constants.ZMANAGER_LOCK);
for (String s : Set.of(Constants.ZGC_LOCK, Constants.ZCOMPACTORS, Constants.ZSSERVERS, Constants.ZTSERVERS)) {
lockPaths = lockPaths.or(path -> path.startsWith(zRoot + s));
}
ctx.getZooCache().clear(lockPaths);
```
You could also define it a predicate that is true if any predicate on a stream is true:
```suggestion
Predicate<String> lockPaths = path -> Stream.of(Constants.ZMANAGER_LOCK, Constants.ZGC_LOCK, Constants.ZCOMPACTORS, Constants.ZSSERVERS, Constants.ZTSERVERS)
.anyMatch(p -> path.startsWith(zRoot + p));
ctx.getZooCache().clear(lockPaths);
```
The setup of the Predicate is a little more costly, but if done once per jvm as a static final, it could be worth it to reduce the number of calls to zooCache.clear(), and it might be a little more readable.
##########
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:
I found some info at this [confluence page](https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=24193420#FAQ-WhathappenstoZKsessionswhiletheclusterisdown?) that says that sessions are preserved while ZK is offline, and sessions (including any session-related data, like ephemeral nodes) are restored and the session timeout refreshed by the new leader after the cluster is restored. So, the premise of this issue is sound, regarding the need to wait for the old accumulo server locks to expire before the restarted services are ready. I assumed all sessions would automatically expire if the ZK cluster were to go down entirely. However, I suppose it makes sense that it's not the case, since that is essentially not any different than if the ZK leader went offline and the cluster had to elect a new leader, which is probably not as uncommon as an entire cluster being offline.
However, the ZooCache watcher should still see the disconnected event, and clears the cache on disconnected, well before the session would have expired. So, even if it never sees the session expired event (which can only happen if ZK never comes back online), I don't think that part of this is relevant. The connection loss KeeperException should happen only for API calls while in a still-disconnected state, but those are separate from the watcher having been triggered and clearing as a result of the disconnected event. Those are something we could potentially address using ZooReader, if it's something we care about. But, it shouldn't affect the timing of ZooCache being updated.
--
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
Re: [PR] Made ITs that restart MAC faster [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion merged PR #5246:
URL: https://github.com/apache/accumulo/pull/5246
--
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
Re: [PR] Made ITs that restart MAC faster [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #5246:
URL: https://github.com/apache/accumulo/pull/5246#discussion_r1915536686
##########
minicluster/src/main/java/org/apache/accumulo/miniclusterImpl/MiniAccumuloClusterImpl.java:
##########
@@ -824,9 +825,46 @@ 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 {
+ 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.
+ boolean startCalled = true;
+ try {
+ getServerContext();
+ } catch (RuntimeException e) {
+ if (e.getMessage().startsWith("Accumulo not initialized")) {
+ startCalled = false;
+ }
+ }
+ if (startCalled) {
+ final ServerContext ctx = getServerContext();
+ final String zRoot = getServerContext().getZooKeeperRoot();
+ ctx.getZooCache().clear((path) -> path.startsWith(zRoot + Constants.ZMANAGER_LOCK));
+ ctx.getZooCache().clear((path) -> path.startsWith(zRoot + Constants.ZGC_LOCK));
+ ctx.getZooCache().clear((path) -> path.startsWith(zRoot + Constants.ZCOMPACTORS));
+ ctx.getZooCache().clear((path) -> path.startsWith(zRoot + Constants.ZSSERVERS));
+ ctx.getZooCache().clear((path) -> path.startsWith(zRoot + Constants.ZTSERVERS));
Review Comment:
Wouldn't this variant test each key in the cache against `Stream.of(Constants.ZMANAGER_LOCK, Constants.ZGC_LOCK, Constants.ZCOMPACTORS, Constants.ZSSERVERS, Constants.ZTSERVERS).anyMatch(p -> path.startsWith(zRoot + p))` ? Wouldn't it create the Stream for every key?
--
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
Re: [PR] Made ITs that restart MAC faster [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #5246:
URL: https://github.com/apache/accumulo/pull/5246#discussion_r1913238004
##########
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:
I believe that a session expired KeeperException will be returned to the ZooKeeper client when the ZooKeeper server is re-started on the same port. But I don't think that watchers will fire when the first ZooKeeper server is terminated, or when the new ZooKeeper server is started. The code does eventually behave correctly, as the tests do complete successfully, but removing these paths from the ZooCache speed things up.
--
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
Re: [PR] Made ITs that restart MAC faster [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #5246:
URL: https://github.com/apache/accumulo/pull/5246#discussion_r1912208519
##########
server/base/src/main/java/org/apache/accumulo/server/util/ZooZap.java:
##########
@@ -90,6 +90,19 @@ public static void main(String[] args) throws Exception {
@Override
public void execute(String[] args) throws Exception {
+ try {
+ var siteConf = SiteConfiguration.auto();
+ // Login as the server on secure HDFS
+ if (siteConf.getBoolean(Property.INSTANCE_RPC_SASL_ENABLED)) {
+ SecurityUtil.serverLogin(siteConf);
+ }
+ zap(siteConf, args);
Review Comment:
Was the change in this file only a refactoring to put the code in a the try block in a method? Seems like no code changes were made besides that but was not sure.
--
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
Re: [PR] Made ITs that restart MAC faster [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Made ITs that restart MAC faster [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #5246:
URL: https://github.com/apache/accumulo/pull/5246#discussion_r1915564121
##########
minicluster/src/main/java/org/apache/accumulo/miniclusterImpl/MiniAccumuloClusterImpl.java:
##########
@@ -824,9 +825,46 @@ 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 {
+ 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.
+ boolean startCalled = true;
+ try {
+ getServerContext();
+ } catch (RuntimeException e) {
+ if (e.getMessage().startsWith("Accumulo not initialized")) {
+ startCalled = false;
+ }
+ }
+ if (startCalled) {
+ final ServerContext ctx = getServerContext();
+ final String zRoot = getServerContext().getZooKeeperRoot();
+ ctx.getZooCache().clear((path) -> path.startsWith(zRoot + Constants.ZMANAGER_LOCK));
+ ctx.getZooCache().clear((path) -> path.startsWith(zRoot + Constants.ZGC_LOCK));
+ ctx.getZooCache().clear((path) -> path.startsWith(zRoot + Constants.ZCOMPACTORS));
+ ctx.getZooCache().clear((path) -> path.startsWith(zRoot + Constants.ZSSERVERS));
+ ctx.getZooCache().clear((path) -> path.startsWith(zRoot + Constants.ZTSERVERS));
Review Comment:
> Wouldn't it create the Stream for every key?
Yeah, I would avoid the second variant. I was just offering inspiration for a possibly more readable version. That second one would perform badly, though. The first variant is slightly less readable, but would perform much better, as there's only a one time cost to set up the chained predicate, especially if the predicate is saved outside the method in some static final.
--
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