Re: [PR] HBASE-29853 Move from async profiler binary to a maven dependency [hbase]

["mnpoonia (via GitHub)" <gi...@apache.org>]

mnpoonia commented on code in PR #7679:
URL: https://github.com/apache/hbase/pull/7679#discussion_r3394255957


##########
hbase-http/src/main/java/org/apache/hadoop/hbase/http/ProfileServlet.java:
##########
@@ -189,109 +265,149 @@ protected void doGet(final HttpServletRequest req, final HttpServletResponse res
     final Integer height = getInteger(req, "height", null);
     final Double minwidth = getMinWidth(req);
     final boolean reverse = req.getParameterMap().containsKey("reverse");
+    int refreshDelay = getInteger(req, "refreshDelay", 0);
+
+    return new ProfileRequest(duration, output, event, interval, jstackDepth, bufsize, thread,
+      simple, width, height, minwidth, reverse, refreshDelay, requestedPid);
+  }
+
+  protected String executeStart(ProfileRequest request, File outputFile) throws IOException {
+    return backend.executeStart(request, outputFile);
+  }
+
+  protected String executeStop(ProfileRequest request, File outputFile) throws IOException {
+    return backend.executeStop(request, outputFile);
+  }
+
+  @Override
+  protected void doGet(final HttpServletRequest req, final HttpServletResponse resp)
+    throws IOException {
+    if (!checkInstrumentationAccess(req, resp)) {
+      return;
+    }
+
+    final ProfileRequest request = parseProfileRequest(req);
+
+    // We keep the pid parameter for backward compatibility but only support profiling this JVM.
+    if (request.getPid() != null && request.getPid().longValue() != currentPid) {
+      writeError(resp, HttpServletResponse.SC_INTERNAL_SERVER_ERROR,
+        "The 'pid' parameter is only supported for the current process when using the "
+          + "embedded async-profiler library.");
+      return;
+    }
+
+    if (profiling) {
+      writeError(resp, HttpServletResponse.SC_INTERNAL_SERVER_ERROR,
+        "Another instance of profiler is already running.");
+      return;
+    }
+
+    int lockTimeoutSecs = 3;
+    boolean locked = false;
+    try {
+      locked = profilerLock.tryLock(lockTimeoutSecs, TimeUnit.SECONDS);
+      if (!locked) {
+        writeError(resp, HttpServletResponse.SC_INTERNAL_SERVER_ERROR,
+          "Unable to acquire lock. Another instance of profiler might be running.");
+        LOG.warn("Unable to acquire lock in " + lockTimeoutSecs
+          + " seconds. Another instance of profiler might be running.");
+        return;
+      }
+
+      File outputFile = createOutputFile(request);
+      // Ensure the file exists so ProfileOutputServlet can poll until it is complete.
+      Files.write(outputFile.toPath(), new byte[0]);
 
-    if (process == null || !process.isAlive()) {
+      executeStart(request, outputFile);
+      profiling = true;
+
+      startStopperThread(request.getDuration(), request, outputFile);
+
+      writeAcceptedResponse(resp, request, outputFile);
+    } catch (InterruptedException e) {

Review Comment:
   ```
   } catch (InterruptedException e) {
       Thread.currentThread().interrupt();     // handled first, specifically
       ...
   } catch (IOException | Error | RuntimeException e) {
       // Catches:
       // - IOException: AsyncProfiler.execute() throws IOException for invalid agent commands
       // - UnsatisfiedLinkError / other Error: native lib absent or incompatible OS/kernel
       // - IllegalStateException / IllegalArgumentException (RuntimeException): double-start,
       //   unsupported event, rejected format from the profiler API
       LOG.warn("Profiler failed to start or execute", e);
       writeError(resp, HttpServletResponse.SC_INTERNAL_SERVER_ERROR,
           "Profiler error: " + e.getMessage()
             + ". Check that the async-profiler native library is compatible with this OS/kernel.");
   ```
   This should cover every scenario. 
   
   ```
   - UnsatisfiedLinkError → caught by Error
   - glibc missing versioned symbols → UnsatisfiedLinkError or Error → caught
   - kernel disabled perf_event → throws at first execute() call → caught by IOException | RuntimeException
   - seccomp policy → same
   - InterruptedException is handled first and explicitly re-interrupts the thread — it never reaches the broader catch
   ```



-- 
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: issues-unsubscribe@hbase.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org