Added ExitCodesIT to test process exit codes under various conditions by dlmarion · Pull Request #5811 · apache/accumulo

dlmarion · 2025-08-20T21:24:55Z

No description provided.

dlmarion · 2025-08-20T21:26:08Z

core/src/main/java/org/apache/accumulo/core/clientImpl/ClientContext.java

-        zooCache.get().close();
-      }
-      if (zooKeeperOpened.get()) {
-        zooSession.get().close();


This needed to be moved to the end because it closes ZooKeeper, which deletes all ephemeral nodes and fires any watchers.

dlmarion · 2025-08-20T21:28:40Z

server/compactor/src/main/java/org/apache/accumulo/compactor/Compactor.java

-      } // end while
-    } catch (Exception e) {
-      LOG.error("Unhandled error occurred in Compactor", e);
-    } finally {


The change here was to remove the finally block. It causes the normal close code to occur even in Exception or Error case.

dlmarion · 2025-08-20T21:31:33Z

test/src/main/java/org/apache/accumulo/test/functional/ExitCodesIT_SimpleSuite.java

+        assertEquals(1, exitValue);
+      }
+    } else {
+      // TODO:


This should be resolved, wasn't quite sure how to fix it.

dlmarion · 2025-08-20T21:32:26Z

Best viewed using the "Hide Whitespace" option.

ctubbsii

Neat strategy for testing exit codes... I'm not sure how easy it's going to be to maintain, as it involves a bit of a learning curve to understand how it works. I'm also wondering if it might be fragile, when we make general improvements. Even so, it might be worth it.

core/src/main/java/org/apache/accumulo/core/clientImpl/ClientContext.java

ctubbsii · 2025-08-20T22:23:34Z

core/src/main/java/org/apache/accumulo/core/lock/ServiceLockSupport.java


      if (acquiredLock) {
-        Halt.halt(-1,
+        Halt.halt(1,


👍 to using positive numbers.

server/base/src/main/java/org/apache/accumulo/server/ServerContext.java

ctubbsii · 2025-08-20T22:27:57Z

server/compactor/src/main/java/org/apache/accumulo/compactor/Compactor.java

-          updateIdleStatus(true);
+    final AtomicReference<Throwable> err = new AtomicReference<>();
+    final LogSorter logSorter = new LogSorter(this);
+    long nextSortLogsCheckTime = System.currentTimeMillis();


Not for this PR, but I wonder if this needs to use the system clock or can be made to use relative time (nanoTime).

ctubbsii · 2025-08-20T22:31:38Z

server/gc/src/main/java/org/apache/accumulo/gc/SimpleGarbageCollector.java

+    // Must set shutdown as completed before calling super.close().
+    // super.close() calls ServerContext.close() ->
+    // ClientContext.close() -> ZooSession.close() which removes
+    // all of the ephemeral nodes and forces the watches to fire.
    getShutdownComplete().set(true);
-    log.info("stop requested. exiting ... ");
-    try {
-      gcLock.unlock();
-    } catch (Exception e) {
-      log.warn("Failed to release GarbageCollector lock", e);
-    }
-
+    super.close();


Is that something that we always do together? If so, I wonder if it can be done inside the close, or if the getShutdownComplete() stuff is needed at all.

ctubbsii · 2025-08-20T22:40:44Z

test/src/main/java/org/apache/accumulo/test/functional/ExitCodesIT_SimpleSuite.java

+          serverClass = TabletServer.class;
+          methodName = "updateIdleStatus";
+          ctorParams = new Class<?>[] {ConfigOpts.class, Function.class, String[].class};
+          ctorArgs = new Object[] {new ConfigOpts(), new ServerContextFunction(),


Can't you inline the ServerContextFunction like the server processes normally do, as in:

Suggested change

ctorArgs = new Object[] {new ConfigOpts(), new ServerContextFunction(),

ctorArgs = new Object[] {new ConfigOpts(), ServerContext::new,

I tried this initially, and I get a compilation error that says "The target type of this expression must be a functional interface"

ctubbsii · 2025-08-20T22:42:12Z

test/src/main/java/org/apache/accumulo/test/functional/ExitCodesIT_SimpleSuite.java

+      // Determine the constructor arguments and parameters for each server class.
+      // Find a method with no-args that does not return anything that is
+      // called during the servers run method that we can intercept to signal
+      // shutdown, exception, or error.


Good comment... but this is wild 😅

test/src/main/java/org/apache/accumulo/test/functional/ExitCodesIT_SimpleSuite.java

dlmarion · 2025-08-21T13:42:34Z

I'm seeing an inconsistency in how we handle Exceptions and Errors in the server threads (AbstractServer.run).

In all other threads the AccumuloUncaughtExceptionHandler will:

Log an error that the thread is dead on an uncaught Exception
Print the stack trace and a message to stderr then halt the VM on an uncaught Error

For critical tasks put into a ThreadPool, we have a method that watches them and throws an ExecutionError if the task has failed. This Error is uncaught and would bubble up to the AccumuloUncaugthExceptionHandler, terminating the VM.

We made some changes recently to AbstractServer in #5796 and #5808 such that a new method AbstractServer.startServer exists because exceptions and errors being throws from AbstractServer.runServer were not being logged. This method looks like:

accumulo/server/base/src/main/java/org/apache/accumulo/server/AbstractServer.java

Lines 78 to 97 in 3a0d8e5

    
           public static void startServer(AbstractServer server, Logger LOG) throws Exception { 
        
             try { 
        
               server.runServer(); 
        
             } catch (Throwable e) { 
        
               System.err 
        
                   .println(server.getClass().getSimpleName() + " died, exception thrown from runServer."); 
        
               e.printStackTrace(); 
        
               LOG.error("{} died, exception thrown from runServer.", server.getClass().getSimpleName(), e); 
        
               throw e; 
        
             } finally { 
        
               try { 
        
                 server.close(); 
        
               } catch (Throwable e) { 
        
                 System.err.println("Exception thrown while closing " + server.getClass().getSimpleName()); 
        
                 e.printStackTrace(); 
        
                 LOG.error("Exception thrown while closing {}", server.getClass().getSimpleName(), e); 
        
                 throw e; 
        
               } 
        
             } 
        
           }

WIth this new method if AbstractServer.runServer completes successfully, then the server process does a normal shutdown finishing with a call to AbstractServer.close. Due to the finally block in AbstractServer.startServer the server's close method will be called again. If AbstractServer.runServer throws an Exception or Error, then the server's close method will be called by the finally block in AbstractServer.startServer, but the other normal shutdown stuff won't occur.

Looking at one of the logs from ExitCodesIT we see the following:

2025-08-21T12:59:37,729 12 [exit_codes.TabletServer_ERROR] INFO : TabletServer_ERROR process shut down.
2025-08-21T12:59:37,729 12 [tserver.TabletServer] ERROR: TabletServer_ERROR died, exception thrown from runServer.
java.lang.StackOverflowError: throwing unknown error
	at org.apache.accumulo.test.functional.exit_codes.TabletServer_ERROR.updateIdleStatus(Unknown Source) ~[?:?]
	at org.apache.accumulo.tserver.TabletServer.run(TabletServer.java:607) ~[classes/:?]
	at org.apache.accumulo.core.trace.TraceWrappedRunnable.run(TraceWrappedRunnable.java:52) ~[classes/:?]
	at java.base/java.lang.Thread.run(Thread.java:829) [?:?]
2025-08-21T12:59:37,733 12 [metrics.MetricsInfoImpl] INFO : Closing metrics registry
2025-08-21T12:59:37,750 12 [server.ServerContext] DEBUG: Shutting down shared executor pool
2025-08-21T12:59:37,750 12 [clientImpl.ClientContext] DEBUG: Closing ZooCache
2025-08-21T12:59:37,750 12 [clientImpl.ClientContext] DEBUG: Closing ZooSession
2025-08-21T12:59:37,752 21 [lock.ServiceLock] DEBUG: [zlock#5c6acefc-a1c0-4fdc-8c81-a7acd162c56e#] zlock#5c6acefc-a1c0-4fdc-8c81-a7acd162c56e#0000000000 was deleted; WatchedEvent state:SyncConnected type:NodeDeleted path:/tservers/TEST/ip-10-113-15-120.evoforge.org:33045/zlock#5c6acefc-a1c0-4fdc-8c81-a7acd162c56e#0000000000 zxid: 827
2025-08-21T12:59:37,754 21 [util.Halt] ERROR: FATAL TABLET_SERVER lost lock (reason = LOCK_DELETED), exiting.

In any other thread, the VM would be halted without closing. I'm thinking that the finally block from AbstractServer.startServer should be removed. Thoughts?

keith-turner · 2025-08-21T19:02:14Z

In any other thread, the VM would be halted without closing. I'm thinking that the finally block from AbstractServer.startServer should be removed. Thoughts?

Are you thinking of something like the following? This would only call close when there is no exception in runServer.

  public static void startServer(AbstractServer server, Logger LOG) throws Exception {
    try {
      server.runServer();
      server.close();
    } catch (Exception e) {
      System.err
          .println(server.getClass().getSimpleName() + " died, exception thrown from runServer.");
      e.printStackTrace();
      LOG.error("{} died, exception thrown from runServer.", server.getClass().getSimpleName(), e);
      throw e;
    }
  }

Or is the call to close() not needed at all in this code because of the comment about AbstractServer.close()?

dlmarion · 2025-08-21T19:08:23Z

Yeah, I don't think server.close is needed. If runServer exits normally without Exception/Error, then it's already been called.

keith-turner · 2025-08-25T20:18:24Z

server/base/src/main/java/org/apache/accumulo/server/AbstractServer.java

      e.printStackTrace();
      LOG.error("{} died, exception thrown from runServer.", server.getClass().getSimpleName(), e);
      throw e;
-    } finally {


A bit futher up could change the catch to catch(Exception e) instead of catch(Throwable e). The only reason it was catching Throwable was in case the finally block threw an exception.

I thought it was for the logging of Error, not just Exception.

It was for logging an error in the case where finally threw an exception in which case the error would be lost and would not propagate. W/o the finally the error will propagate now. So it depends on what the outer code does with it.

Fixed in ec5cd1d

Do we still want to catch an exception and log here to achieve the goals of #5796? I was commenting about catching Throwable because it was only added in #5808 because of the finally. So not sure we need to catch Throwable, but may still want to catch exception.

I don't think we need to. I checked the logs and the Exception and Error are logged correctly. The issue in #5796 was an exception in the try-with-resources was being suppressed when an exception happened in the close. try-with-resources is no longer being used.

keith-turner · 2025-08-25T20:23:28Z

server/base/src/main/java/org/apache/accumulo/server/AbstractServer.java

+
+    context.getLowMemoryDetector().logGCInfo(getConfiguration());
+
+    // Must set shutdown as completed before calling super.close().


This comment mentions calling super.close(), but do not see that.

Updated comment in ec5cd1d

keith-turner · 2025-08-25T20:37:05Z

server/compactor/src/main/java/org/apache/accumulo/compactor/Compactor.java

            }
+            continue;
+          } finally {
+            currentCompactionId.set(null);


Does not seem like this should be cleared here because its used for dead compaction detection and it should be set for the entire time that a compactor is running a compaction.

Yep, good catch.

Removed finally block in ec5cd1d

keith-turner · 2025-08-25T21:00:27Z

server/compactor/src/main/java/org/apache/accumulo/compactor/Compactor.java

+      LOG.warn("Failed to close filesystem : {}", e.getMessage(), e);
    }

+    super.close();


Seems like the the new pattern in this PR is that super.close() is always called at the end of each run() method. Wondering if would make sense to push this into AbstractServer like the following. This would centralize the pattern to one place in the code.

public void runServer() throws Exception { final AtomicReference<Throwable> err = new AtomicReference<>(); serverThread = new Thread(TraceUtil.wrap(()->{ this.run(); close(); }), applicationName);

Implemented in ec5cd1d

keith-turner · 2025-08-25T21:04:01Z

test/src/main/java/org/apache/accumulo/test/functional/ExitCodesIT.java

+          compactorArgs.add("-o");
+          compactorArgs.add(Property.COMPACTOR_GROUP_NAME.getKey() + "=TEST");
+          serverClass = Compactor.class;
+          methodName = "updateIdleStatus";


What happens w/ the test if this method does not exists? Does the test fail?
Seems like maybe it would fail because the SHUTDOWN case would expect an exit code of zero but would see some other exit code. But not sure.

Wondering if instead of this run time overriding if it would be possible to do it at compile time instead w/ something like the following. Then if someone changes a method name they will get a compile time error.

class ExitCompactor extends Compactor { private final TerminalBehavior terminalBehavior; protected ExitCompactor(ConfigOpts opts, String[] args, TerminalBehavior terminalBehavior) { super(opts, args); this.terminalBehavior = terminalBehavior; } @Override public void updateIdleStatus(boolean idle){ switch (terminalBehavior){ case SHUTDOWN: super.requestShutdownForTests(); break; case ERROR: throw new StackOverflowError(); case EXCEPTION: throw new RuntimeException(); } } }

What happens w/ the test if this method does not exists? Does the test fail?
Seems like maybe it would fail because the SHUTDOWN case would expect an exit code of zero but would see some other exit code. But not sure.

I can make it fail explicity by adding the following to ProcessProxy.main:

diff --git a/test/src/main/java/org/apache/accumulo/test/functional/ExitCodesIT.java b/test/src/main/java/org/apache/accumulo/test/functional/ExitCodesIT.java index 4f8f948d26..c17a30ab6e 100644 --- a/test/src/main/java/org/apache/accumulo/test/functional/ExitCodesIT.java +++ b/test/src/main/java/org/apache/accumulo/test/functional/ExitCodesIT.java @@ -155,6 +155,15 @@ public class ExitCodesIT extends SharedMiniClusterBase { throw new UnsupportedOperationException(st + " is not currently supported"); } + // Check that methodName exists on serverClass + try { + @SuppressWarnings("unused") + var ignored = serverClass.getDeclaredMethod(methodName); + } catch (NoSuchMethodException nsme) { + nsme.printStackTrace(); + System.exit(42); + } +

Wondering if instead of this run time overriding if it would be possible to do it at compile time instead w/ something like the following. Then if someone changes a method name they will get a compile time error.

I think I could make something that is functionally equivalent without using ByteBuddy, but it would be more code. I started with ByteBuddy initially because I wasn't sure of the design when I started, but I knew that I basically needed a proxy for each server class.

I think I could make something that is functionally equivalent without using ByteBuddy, but it would be more code.

If its not much more code then there are benefits. Can fully navigate and manipulate the code in an IDE and easily see what the test references. Also if a method is renamed will get a quick compile time failure instead of this test failing and then trying to figure out why.

Removed ByteBuddy and subclassed server processes in ec5cd1d

keith-turner · 2025-08-27T17:28:15Z

test/src/main/java/org/apache/accumulo/test/functional/ExitCodesIT.java

+      // We need to let this time out and then
+      // terminate the process.
+      IllegalStateException ise = assertThrows(IllegalStateException.class,
+          () -> Wait.waitFor(() -> !pi.getProcess().isAlive(), 120_000));


Does this always wait for 2 mins? If so, could we lower time to wait to 30s or 60s?

I can change to 1m. The other two tests for the GC take 30-35s

dlmarion added 3 commits August 20, 2025 20:49

Added ExitCodesIT and related changes

99c8070

Merge branch 'main' into exit-codes-it

87e8507

Fixed Manager test

5043c2e

dlmarion added this to the 4.0.0 milestone Aug 20, 2025

dlmarion requested a review from ctubbsii August 20, 2025 21:24

dlmarion self-assigned this Aug 20, 2025

dlmarion commented Aug 20, 2025

View reviewed changes

ctubbsii reviewed Aug 20, 2025

View reviewed changes

dlmarion added 3 commits August 21, 2025 12:06

Merge branch 'main' into exit-codes-it

f855c82

Implemented PR suggestions

d41dbcc

Resolve issue with ScanServer by removing finally block

9ae6f1d

dlmarion added 3 commits August 25, 2025 16:55

Removed finally block in AbstractServer.startServer that called close

bf5e3be

Moved common close calls into AbstractServer, updated Halt calls

1337d84

Fix SpotBugs and logic issue in AbstractServer.close

57e6330

keith-turner reviewed Aug 25, 2025

View reviewed changes

Addressed PR comments

ec5cd1d

keith-turner approved these changes Aug 27, 2025

View reviewed changes

Changed GC wait time from 2m to 1m

6259885

dlmarion merged commit 501efad into apache:main Aug 27, 2025
8 checks passed

dlmarion deleted the exit-codes-it branch August 27, 2025 17:53

	ctorArgs = new Object[] {new ConfigOpts(), new ServerContextFunction(),
	ctorArgs = new Object[] {new ConfigOpts(), ServerContext::new,


		context.getLowMemoryDetector().logGCInfo(getConfiguration());

		// Must set shutdown as completed before calling super.close().

Conversation

dlmarion commented Aug 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dlmarion commented Aug 20, 2025

Uh oh!

ctubbsii left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dlmarion commented Aug 21, 2025

Uh oh!

keith-turner commented Aug 21, 2025

Uh oh!

dlmarion commented Aug 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects