From 9b0818b535207d5c9ee2e007548ed3ad04ac4c7f Mon Sep 17 00:00:00 2001 From: LSmyrnaios Date: Thu, 14 Mar 2024 13:59:23 +0200 Subject: [PATCH] - Add handling for additional/specific exceptions, when checking the "futures". - Move common "ExecutionException" handling-code into its own method: "GenericUtils.getSelectedStackTraceForCausedException()". - Avoid a double log. - Code polishing. --- .../components/ScheduledTasks.java | 14 +++------ .../services/BulkImportServiceImpl.java | 12 +++----- .../urls_controller/util/GenericUtils.java | 19 ++++++++++++ .../util/ParquetFileUtils.java | 30 +++++++++---------- 4 files changed, 41 insertions(+), 34 deletions(-) diff --git a/src/main/java/eu/openaire/urls_controller/components/ScheduledTasks.java b/src/main/java/eu/openaire/urls_controller/components/ScheduledTasks.java index 2f69e95..f5901f5 100644 --- a/src/main/java/eu/openaire/urls_controller/components/ScheduledTasks.java +++ b/src/main/java/eu/openaire/urls_controller/components/ScheduledTasks.java @@ -112,22 +112,16 @@ public class ScheduledTasks { if ( ! future.get() ) // Get and see if an exception is thrown. This blocks the current thread, until the task of the future has finished. numFailedTasks ++; } catch (ExecutionException ee) { // These can be serious errors like an "out of memory exception" (Java HEAP). - // The stacktrace of the "ExecutionException" is the one of the current code and not the code which ran inside the background-task. Try to get the cause. - Throwable throwable = ee.getCause(); - if ( throwable == null ) { - logger.warn("No cause was retrieved for the \"ExecutionException\"!"); - throwable = ee; - } - logger.error(GenericUtils.getSelectiveStackTrace(throwable, "Background task_" + i + " failed with: " + throwable.getMessage(), 30)); + logger.error(GenericUtils.getSelectedStackTraceForCausedException(ee, "Background_task_" + i + " failed with: ", null, 30)); numFailedTasks ++; } catch (CancellationException ce) { - logger.error("Background task_" + i + " was cancelled: " + ce.getMessage()); + logger.error("Background_task_" + i + " was cancelled: " + ce.getMessage()); numFailedTasks ++; } catch (InterruptedException ie) { - logger.error("Background task_" + i + " was interrupted: " + ie.getMessage()); + logger.error("Background_task_" + i + " was interrupted: " + ie.getMessage()); numFailedTasks ++; } catch (IndexOutOfBoundsException ioobe) { - logger.error("IOOBE for background task_" + i + " in the futures-list! " + ioobe.getMessage()); + logger.error("IOOBE for background_task_" + i + " in the futures-list! " + ioobe.getMessage()); // Only here, the "future" will be null. } finally { if ( future != null ) // It may be null in case we have a IOBE. diff --git a/src/main/java/eu/openaire/urls_controller/services/BulkImportServiceImpl.java b/src/main/java/eu/openaire/urls_controller/services/BulkImportServiceImpl.java index 056a722..e0c38b4 100644 --- a/src/main/java/eu/openaire/urls_controller/services/BulkImportServiceImpl.java +++ b/src/main/java/eu/openaire/urls_controller/services/BulkImportServiceImpl.java @@ -164,18 +164,14 @@ public class BulkImportServiceImpl implements BulkImportService { // In case all the files failed to be bulk-imported, then we will detect it in the "numSuccessfulSegments"-check later. // The failed-to-be-imported files, will not be deleted, even if the user specifies that he wants to delete the directory. } catch (ExecutionException ee) { // These can be serious errors like an "out of memory exception" (Java HEAP). - // The stacktrace of the "ExecutionException" is the one of the current code and not the code which ran inside the background-task. Try to get the cause. - Throwable throwable = ee.getCause(); - if ( throwable == null ) { - logger.warn("No cause was retrieved for the \"ExecutionException\"!"); - throwable = ee; - } - String stackTraceMessage = GenericUtils.getSelectiveStackTrace(throwable, "Task_" + i + " failed with: " + throwable.getMessage() + additionalLoggingMsg, 15); numFailedSegments ++; - logger.error("Task_" + i + " failed with: " + stackTraceMessage); + logger.error(GenericUtils.getSelectedStackTraceForCausedException(ee, "Task_" + i + " failed with: ", additionalLoggingMsg, 15)); } catch (CancellationException ce) { numFailedSegments ++; logger.error("Task_" + i + " was cancelled: " + ce.getMessage() + additionalLoggingMsg); + } catch (InterruptedException ie) { + numFailedSegments ++; + logger.error("Task_" + i + " was interrupted: " + ie.getMessage()); } catch (IndexOutOfBoundsException ioobe) { logger.error("IOOBE for task_" + i + " in the futures-list! " + ioobe.getMessage() + additionalLoggingMsg); } diff --git a/src/main/java/eu/openaire/urls_controller/util/GenericUtils.java b/src/main/java/eu/openaire/urls_controller/util/GenericUtils.java index 876892e..cea2f3d 100644 --- a/src/main/java/eu/openaire/urls_controller/util/GenericUtils.java +++ b/src/main/java/eu/openaire/urls_controller/util/GenericUtils.java @@ -1,11 +1,16 @@ package eu.openaire.urls_controller.util; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + import javax.servlet.http.HttpServletRequest; import java.text.SimpleDateFormat; import java.util.Date; public class GenericUtils { + private static final Logger logger = LoggerFactory.getLogger(GenericUtils.class); + public static final String endOfLine = "\n"; @@ -22,6 +27,20 @@ public class GenericUtils { } + + public static String getSelectedStackTraceForCausedException(Throwable thr, String firstMessage, String additionalMessage, int numOfLines) + { + // The stacktrace of the "ExecutionException" is the one of the current code and not the code which ran inside the background-task. Try to get the cause. + Throwable causedThrowable = thr.getCause(); + if ( causedThrowable == null ) { + logger.warn("No cause was retrieved for the \"ExecutionException\"!"); + causedThrowable = thr; + } + String initialMessage = firstMessage + causedThrowable.getMessage() + ((additionalMessage != null) ? additionalMessage : ""); + return getSelectiveStackTrace(causedThrowable, initialMessage, numOfLines); + } + + public static String getSelectiveStackTrace(Throwable thr, String initialMessage, int numOfLines) { StackTraceElement[] stels = thr.getStackTrace(); diff --git a/src/main/java/eu/openaire/urls_controller/util/ParquetFileUtils.java b/src/main/java/eu/openaire/urls_controller/util/ParquetFileUtils.java index c2a5db7..d8184e3 100644 --- a/src/main/java/eu/openaire/urls_controller/util/ParquetFileUtils.java +++ b/src/main/java/eu/openaire/urls_controller/util/ParquetFileUtils.java @@ -43,6 +43,7 @@ import java.util.ArrayList; import java.util.Base64; import java.util.List; import java.util.concurrent.Callable; +import java.util.concurrent.CancellationException; import java.util.concurrent.ExecutionException; import java.util.concurrent.Future; import java.util.stream.Collectors; @@ -687,11 +688,11 @@ public class ParquetFileUtils { int numOfAllPayloadParquetFileCreations = 0; int numOfFailedPayloadParquetFileCreations = 0; - for ( Future future : futures ) + int sizeOfFutures = futures.size(); + for ( int i = 0; i < sizeOfFutures; ++i ) { - ParquetReport parquetReport = null; try { - parquetReport = future.get(); + ParquetReport parquetReport = futures.get(i).get(); boolean hasProblems = (! parquetReport.isSuccessful()); ParquetReport.ParquetType parquetType = parquetReport.getParquetType(); if ( parquetType.equals(ParquetReport.ParquetType.attempt) ) { @@ -707,20 +708,17 @@ public class ParquetFileUtils { logger.error(errMsg); return new SumParquetSuccess(false, false, ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR).body(errMsg)); } - } catch (Exception e) { - Throwable throwable = e; - if ( e instanceof ExecutionException ) { - // The stacktrace of the "ExecutionException" is the one of the current code and not the code which ran inside the background-task. Try to get the cause. - throwable = e.getCause(); - if ( throwable == null ) { - logger.warn("No cause was retrieved for the \"ExecutionException\"!"); - throwable = e; - } - } - logger.error("", throwable); - // We do not know if the failed "future" refers to a "payload" or to a "attempt". - // So we cannot increase a specific counter. That's ok, the only drawback if that we may try to "load" the non-existent data and get an exception. + } catch (ExecutionException ee) { // These can be serious errors like an "out of memory exception" (Java HEAP). + logger.error(GenericUtils.getSelectedStackTraceForCausedException(ee, "Parquet_task_" + i + " failed with: ", null, 15)); + } catch (CancellationException ce) { + logger.error("Parquet_task_" + i + " was cancelled: " + ce.getMessage()); + } catch (InterruptedException ie) { + logger.error("Parquet_task_" + i + " was interrupted: " + ie.getMessage()); + } catch (IndexOutOfBoundsException ioobe) { + logger.error("IOOBE for parquet_task_" + i + " in the futures-list! " + ioobe.getMessage()); } + // We do not know if the failed "future" refers to a "payload" or to a "attempt". + // So we cannot increase a specific counter. That's ok, the only drawback if that we may try to "load" the non-existent data and get an exception. } // End-for boolean hasAttemptParquetFileProblem = (numOfFailedAttemptParquetFileCreations == numOfAllAttemptParquetFileCreations);