[GitHub] [accumulo] ctubbsii commented on a change in pull request #2414: Fix issues in MiniAccumuloCluster.verifyUp that is causing tests to fail

[GitBox <gi...@apache.org>]

ctubbsii commented on a change in pull request #2414:
URL: https://github.com/apache/accumulo/pull/2414#discussion_r787999098



##########
File path: minicluster/src/main/java/org/apache/accumulo/miniclusterImpl/MiniAccumuloClusterImpl.java
##########
@@ -624,53 +640,80 @@ private void verifyUp() throws InterruptedException, IOException {
           "Error starting TabletServer " + tsExpectedCount + "- instance not started");
     }
 
-    var zrw = getServerContext().getZooReaderWriter();
-    String rootPath = getServerContext().getZooKeeperRoot();
+    Watcher w = new Watcher() {
+      public void process(WatchedEvent event) {
+        if (event.getState() == KeeperState.Expired) {
+          log.debug("Session expired, state of current session : {}", event.getState());
+        }
+      }
+    };

Review comment:
       I don't think we need to register a watcher. This complicates things.

##########
File path: minicluster/src/main/java/org/apache/accumulo/miniclusterImpl/MiniAccumuloClusterImpl.java
##########
@@ -624,53 +640,80 @@ private void verifyUp() throws InterruptedException, IOException {
           "Error starting TabletServer " + tsExpectedCount + "- instance not started");
     }
 
-    var zrw = getServerContext().getZooReaderWriter();
-    String rootPath = getServerContext().getZooKeeperRoot();
+    Watcher w = new Watcher() {
+      public void process(WatchedEvent event) {
+        if (event.getState() == KeeperState.Expired) {
+          log.debug("Session expired, state of current session : {}", event.getState());
+        }
+      }
+    };
+    try (ZooKeeper zk = new ZooKeeper(getZooKeepers(), 60000, w)) {
 
-    int tsActualCount = 0;
-    int tryCount = 0;
-    try {
-      while (tsActualCount != tsExpectedCount) {
-        tryCount++;
-        tsActualCount = 0;
-        for (String child : zrw.getChildren(rootPath + Constants.ZTSERVERS)) {
-          tsActualCount++;
-          if (zrw.getChildren(rootPath + Constants.ZTSERVERS + "/" + child).isEmpty())
-            log.info("TServer " + tsActualCount + " not yet present in ZooKeeper");
+      String secret = getSiteConfiguration().get(Property.INSTANCE_SECRET);
+
+      for (int i = 0; i < numTries; i++) {
+        if (zk.getState().equals(States.CONNECTED)) {
+          zk.addAuthInfo("digest", ("accumulo" + ":" + secret).getBytes(UTF_8));

Review comment:
       We should have a static utility method for constructing the auth info, so we don't have to do this more than one place.

##########
File path: minicluster/src/main/java/org/apache/accumulo/miniclusterImpl/MiniAccumuloClusterImpl.java
##########
@@ -327,6 +334,13 @@ public MiniAccumuloClusterImpl(MiniAccumuloConfigImpl config) throws IOException
 
     this.config = config.initialize();
 
+    if (Boolean.TRUE.equals(Boolean
+        .valueOf(this.config.getSiteConfig().get(Property.TSERV_NATIVEMAP_ENABLED.name())))) {
+      if (!NativeMap.isLoaded())
+        throw new RuntimeException(
+            "MAC configured to use native maps, but unable to load the library.");
+    }
+

Review comment:
       I wouldn't expect this to work. The native maps are loaded by adding LD_LIBRARY_PATH environment to the tserver Process before it is launched. There's no reason to expect the native map to be loaded inside the MiniAccumuloClusterImpl... only for the processes it launches.
   
   (Also, no need to do `if (Boolean.TRUE.equals(someBoolExpr))` when you can just do `if (someBoolExpr)`)




-- 
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