[GitHub] [flink] GJL commented on a change in pull request #11994: [FLINK-17514] Let TaskCancelerWatchDog call TaskManagerActions.notifyFatalError with non-null cause

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

GJL commented on a change in pull request #11994:
URL: https://github.com/apache/flink/pull/11994#discussion_r420139082



##########
File path: flink-runtime/src/test/java/org/apache/flink/runtime/taskmanager/TaskTest.java
##########
@@ -855,7 +862,8 @@ public void testFatalErrorOnCanceling() throws Exception {
 			task.cancelExecution();
 
 			// wait for the notification of notifyFatalError
-			taskManagerActions.latch.await();
+			final Throwable fatalError = fatalErrorFuture.join();
+			assertThat(fatalError, instanceOf(fatalErrorType));
 		} catch (Throwable t) {
 			fail("No exception is expected to be thrown by fatal error handling");

Review comment:
       Explicitly failing the test seems unnecessarily verbose. 

##########
File path: flink-runtime/src/main/java/org/apache/flink/runtime/taskmanager/Task.java
##########
@@ -1552,8 +1553,7 @@ public void run() {
 
 				if (executerThread.isAlive()) {
 					String msg = "Task did not exit gracefully within " + (timeoutMillis / 1000) + " + seconds.";
-					log.error(msg);

Review comment:
       `log` is now unused.

##########
File path: flink-runtime/src/main/java/org/apache/flink/runtime/taskmanager/Task.java
##########
@@ -1557,8 +1557,7 @@ public void run() {
 				}
 			}
 			catch (Throwable t) {

Review comment:
       Is it even necessary to catch `Throwable` and rethrow it as a `FlinkRuntimeException`? We are already handling all checked exceptions. Therefore the code would compile without the `try-catch` block and even do something similar.

##########
File path: flink-core/src/main/java/org/apache/flink/util/ExceptionUtils.java
##########
@@ -132,31 +132,39 @@ public static boolean isJvmFatalOrOutOfMemoryError(Throwable t) {
 	}
 
 	/**
-	 * Generates new {@link OutOfMemoryError} with more detailed message.
+	 * Tries to enrich the passed exception with additional information.
 	 *
 	 * <p>This method improves error message for direct and metaspace {@link OutOfMemoryError}.
 	 * It adds description of possible causes and ways of resolution.
 	 *
 	 * @param exception The exception to enrich.
 	 * @return either enriched exception if needed or the original one.
 	 */
-	public static Throwable enrichTaskManagerOutOfMemoryError(Throwable exception) {
-		if (isMetaspaceOutOfMemoryError(exception)) {
-			return changeOutOfMemoryErrorMessage(exception, TM_METASPACE_OOM_ERROR_MESSAGE);
-		} else if (isDirectOutOfMemoryError(exception)) {
-			return changeOutOfMemoryErrorMessage(exception, TM_DIRECT_OOM_ERROR_MESSAGE);
+	public static Throwable tryEnrichTaskManagerError(Throwable exception) {

Review comment:
       Maybe add `@Nullable`




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

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