YARN-6302 Fail the node, if Linux Container Executor is not configured properly by miklosszegedi · Pull Request #200 · apache/hadoop

miklosszegedi · 2017-03-08T20:47:22Z

YARN-6302 Fail the node, if Linux Container Executor is not configured properly

Change-Id: Ia676061fd49cc7f54dbd9ae22bb999d4ea8a965b

Change-Id: Ib17d4a357b6fdf1a6d940f0641770054f1f73e81

Change-Id: Ib1e7215f9dac6825bda2eb54707782c59f19eb0c

templedf

Thanks for the patch!

templedf · 2017-03-20T22:05:04Z

        logOutput(diagnostics);
        container.handle(new ContainerDiagnosticsUpdateEvent(containerId,
            diagnostics));
+        if (exitCode == LinuxContainerExecutorExitCode.


Would it be cleaner to create a new LinuxContainerExecutorExitCode from your exitCode and then test via ==?

I get enum types cannot be instantiated. I could create a function that returns the appropriate enum for an int value, but would not that be an overkill here?

Right. Forgot about that. You'd basically have to recreate the same code in the enum to get an instance from an int.

Maybe add an equals() method to the enum that can compare against ints as well? Maybe not worth it. Just shortening the enum name may be enough...

templedf · 2017-03-20T22:06:05Z

+import org.apache.hadoop.classification.InterfaceStability.Unstable;
+
+/**
+ * This exception is thrown on unrecoverable container launch errors.


No reason to constrain the use of the exception. Maybe offer the launch errors as an example or suggested use?

Agreed. Fixed the code.

templedf · 2017-03-20T22:06:17Z


+  /**
+   * The container exit code.
+   */


templedf · 2017-03-20T22:07:04Z

  private NodeHealthScriptRunner nodeHealthScriptRunner;
  private LocalDirsHandlerService dirsHandler;
+  private Exception nodeHealthException;
+  long nodeHealthExceptionReportTime;


My rule of thumb is that If it's not private, it should have javadocs.

My mistake. Fixed.

templedf · 2017-03-20T22:08:41Z

-    boolean scriptHealthStatus = (nodeHealthScriptRunner == null) ? true
-        : nodeHealthScriptRunner.isHealthy();
-    return scriptHealthStatus && dirsHandler.areDisksHealthy();
+    boolean scriptHealthStatus = nodeHealthScriptRunner == null ||


Maybe rename this one scriptHealthy

templedf · 2017-03-20T22:09:47Z

@@ -80,6 +97,7 @@ long getLastHealthReportTime() {
    long lastReportTime = (nodeHealthScriptRunner == null)


This isn't your code, but it's hideous. Wanna clean it up, too? :)

templedf · 2017-03-20T22:11:56Z

          .setContainerLocalDirs(containerLocalDirs)
          .setContainerLogDirs(containerLogDirs).build());
+    } catch (ConfigurationException e) {
+      LOG.error("Failed to launch container.", e);


Since you know it was a configuration error, you may as well say so in the error message.

It will be redundant, since the exception type is usually visible, but I fixed it.

templedf · 2017-03-20T22:12:41Z

          .setContainerLogDirs(containerLogDirs)
          .build());
+    } catch (ConfigurationException e) {
+      LOG.error("Failed to relaunch container.", e);


Since you know it was a configuration error, you may as well say so in the error message.

It will be redundant, since the exception type is usually visible, but I fixed it.

templedf · 2017-03-20T22:31:33Z

  INVALID_ARGUMENT_NUMBER = 1,
-  INVALID_USER_NAME, //2
-  INVALID_COMMAND_PROVIDED, //3
+  //INVALID_USER_NAME 2


This section of code makes me want to weep.

INVALID_USER_NAME was forgotten earlier, so I removed it, and I just followed the pattern that is in the code right now keeping the original value commented.
If we want to refactor this right now, I would generate large pseudorandom number do be able to check the difference and be able to search for the error code like a GUID in a search engine.

Yeah, I didn't mean it was your fault. Salvaging this code isn't your problem. :)

templedf · 2017-03-20T23:53:24Z

   * @return the reporting string of health of the node
   */
  String getHealthReport() {
+    String healthReport = "";


This would be a bit cleaner with a Joiner:

String scriptReport = (nodeHealthScriptRunner == null) ? null : nodeHealthScriptRunner.getHealthReport();
String discReport = dirsHandler.getDisksHealthReport(false);
String exceptionReport = nodeHealthException == null ? null : nodeHealthException.getMessage();
String healthReport = Joiner.on(SEPARATOR).skipNulls().join(scriptReport, discReport.equals("") ? null : discReport, exceptionReport);

The discReport throws a monkey wrench in the works because it's returning "" instead of null. There's probably a more elegant solution that what I did above...

templedf · 2017-03-21T18:46:50Z

+ * This exception is thrown on unrecoverable configuration errors.
+ * An example is container launch error due to configuration.
 */
 @Public


Maybe make it evolving?

templedf · 2017-03-21T20:28:57Z

  INVALID_ARGUMENT_NUMBER = 1,
-  INVALID_USER_NAME, //2
-  INVALID_COMMAND_PROVIDED, //3
+  //INVALID_USER_NAME 2


Yeah, I didn't mean it was your fault. Salvaging this code isn't your problem. :)

templedf · 2017-03-21T20:29:32Z

+ * This exception is thrown on unrecoverable configuration errors.
+ * An example is container launch error due to configuration.
+ */
+@Public


Should this be @evolving?

templedf · 2017-03-21T20:35:43Z


+  /**
+   * The container exit code.
+   */


Since this is an inner class of LCE, you can safely drop the LCE from the enum name, which will make the subsequent code less messy.

templedf · 2017-03-21T20:37:20Z

        logOutput(diagnostics);
        container.handle(new ContainerDiagnosticsUpdateEvent(containerId,
            diagnostics));
+        if (exitCode == LinuxContainerExecutorExitCode.


Right. Forgot about that. You'd basically have to recreate the same code in the enum to get an instance from an int.

Maybe add an equals() method to the enum that can compare against ints as well? Maybe not worth it. Just shortening the enum name may be enough...

templedf · 2017-03-24T19:54:33Z

        container.handle(new ContainerDiagnosticsUpdateEvent(containerId,
            diagnostics));
-        if (exitCode == LinuxContainerExecutorExitCode.
+        if (exitCode == ExitCode.


Sorry to pick, but can we split these lines on the == instead of the . ?

What am I missing? It doesn't look like anything changed...

I did not run my last git push. It should be fixed now.

templedf · 2017-03-24T19:55:29Z

        LOG.info(
            "Container was marked as inactive. Returning terminated error");
-        return ExitCode.TERMINATED.getExitCode();
+        return ContainerExecutor.ExitCode.TERMINATED.getExitCode();


I don't think this is needful, but you can do it if you want.

There is one ExitCode now in this class as well.

Ah, missed that.

Miklos Szegedi added 3 commits March 7, 2017 14:39

YARN-6302 Throw on error 24

cb97a19

Change-Id: Ia676061fd49cc7f54dbd9ae22bb999d4ea8a965b

YARN-6302 Shutdown on error 24

6f7872e

Change-Id: Ib17d4a357b6fdf1a6d940f0641770054f1f73e81

YARN-6302 Mark node unhealthy on error 24

03f4cd8

Change-Id: Ib1e7215f9dac6825bda2eb54707782c59f19eb0c

templedf reviewed Mar 21, 2017

View reviewed changes

YARN-6302 Reflecting on comments

c3e9721

templedf reviewed Mar 21, 2017

View reviewed changes

YARN-6302 Reflecting on comments

8eb0fc1

templedf reviewed Mar 24, 2017

View reviewed changes

Miklos Szegedi added 2 commits March 24, 2017 19:22

YARN-6302 Minor indentation changes

d012be4

YARN-6302 Fix Jenkins issues

0aa9560

asfgit force-pushed the trunk branch from 25ac447 to 4d1fac5 Compare April 15, 2017 19:10

miklosszegedi closed this Mar 5, 2018

This was referenced Sep 18, 2026

HADOOP-19990: Upgrade to Bouncycastle 1.86 due to CVEs #8743

Open

HADOOP-7856. Improve DiskChecker javadocs. #8746

Open

YARN-11989. Disable Jersey WADL generation on the Timeline Server #8745

Open

		@@ -80,6 +97,7 @@ long getLastHealthReportTime() {
		long lastReportTime = (nodeHealthScriptRunner == null)

Conversation

miklosszegedi commented Mar 8, 2017

Uh oh!

templedf left a comment

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!

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!

Reviewers

Assignees