GOBBLIN-875: Emit container health metrics when running in cluster mode #2729

sv2000 · 2019-09-09T17:27:48Z

Dear Gobblin maintainers,

Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below!

JIRA

My PR addresses the following Gobblin JIRA issues and references them in the PR title. For example, "[GOBBLIN-XXX] My Gobblin PR"
- https://issues.apache.org/jira/browse/GOBBLIN-875

Description

Here are some details about my PR, including screenshots (if applicable):
This task implements a service that emits CPU/Memory health metrics from the JVM when running in the cluster mode.

Tests

My PR adds the following unit tests OR does not need testing for this extremely good reason:
Added unit test in ContainerHealthMetricsServiceTest.

Commits

My commits all reference JIRA issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
1. Subject is separated from body by a blank line
2. Subject is limited to 50 characters
3. Subject does not end with a period
4. Subject uses the imperative mood ("add", not "adding")
5. Body wraps at 72 characters
6. Body explains "what" and "why", not "how"

autumnust · 2019-09-09T17:48:19Z

gobblin-api/src/main/java/org/apache/gobblin/configuration/ConfigurationKeys.java

@@ -954,4 +954,5 @@
   */
  public static final String AVRO_SCHEMA_CHECK_STRATEGY = "avro.schema.check.strategy";
  public static final String AVRO_SCHEMA_CHECK_STRATEGY_DEFAULT = "org.apache.gobblin.util.schema_check.AvroSchemaCheckDefaultStrategy";
+


Empty line, another one inGobblinApplicationMaster.java, line 116

autumnust · 2019-09-09T18:01:01Z

gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinTaskRunner.java

@@ -196,6 +196,11 @@ public GobblinTaskRunner(String applicationName,
    this.services.addAll(suite.getServices());

    this.services.addAll(getServices());
+
+    if (ConfigUtils.getBoolean(this.config, GobblinClusterConfigurationKeys.CONTAINER_HEALTH_METRICS_SERVICE_ENABLED, false)) {


Should we package this service as part of TaskRunnerSuiteBase instead of having a this.services.addAll(getServices) plus another service which is used for metric-reporting outside?

The TaskRunnerSuiteBase is an abstract class with two implementations, process model and thread model. Wanted to leave getServices() in TaskRunnerSuiteBase as an abstract method so as not to change the contract of the class.

Sorry I should not mentioned TaskRunnerSuiteBase. What I meant to say is, does it make more sense to add this service inside getServices method in GobblinTaskRunner ?

The comment of getServices is :
Creates and returns a {@link List} of additional {@link Service}s that should be run in this {@link GobblinTaskRunner}. Sub-classes that need additional {@link Service}s to run, should override this method

codecov-io · 2019-09-09T18:02:51Z

Codecov Report

Merging #2729 into master will increase coverage by 0.08%.
The diff coverage is 70.42%.

@@             Coverage Diff              @@
##             master    #2729      +/-   ##
============================================
+ Coverage     44.99%   45.08%   +0.08%     
- Complexity     8742     8759      +17     
============================================
  Files          1884     1886       +2     
  Lines         70295    70377      +82     
  Branches       7715     7718       +3     
============================================
+ Hits          31629    31726      +97     
+ Misses        35735    35709      -26     
- Partials       2931     2942      +11

Impacted Files	Coverage Δ	Complexity Δ
...bblin/cluster/GobblinClusterConfigurationKeys.java	`0% <ø> (ø)`	`0 <0> (ø)`	⬇️
...che/gobblin/yarn/GobblinYarnConfigurationKeys.java	`0% <ø> (ø)`	`0 <0> (ø)`	⬇️
...apache/gobblin/cluster/ContainerHealthMetrics.java	`0% <0%> (ø)`	`0 <0> (?)`
.../apache/gobblin/cluster/GobblinClusterManager.java	`53.91% <0%> (-0.51%)`	`27 <0> (ø)`
...org/apache/gobblin/yarn/GobblinYarnTaskRunner.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...a/org/apache/gobblin/aws/GobblinAWSTaskRunner.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
.../apache/gobblin/yarn/GobblinApplicationMaster.java	`18.84% <100%> (+2.42%)`	`3 <0> (ø)`	⬇️
...n/java/org/apache/gobblin/yarn/YarnHelixUtils.java	`20% <100%> (+2.75%)`	`3 <1> (+1)`	⬆️
.../org/apache/gobblin/cluster/GobblinTaskRunner.java	`64.35% <50%> (-0.91%)`	`28 <1> (-1)`
...gobblin/cluster/ContainerHealthMetricsService.java	`78.57% <78.57%> (ø)`	`5 <5> (?)`
... and 18 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a409908...621c530. Read the comment docs.

shirshanka

mostly minor comments / questions.

shirshanka · 2019-09-09T22:03:19Z

gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinClusterConfigurationKeys.java

@@ -167,4 +167,5 @@

  public static final String HELIX_JOB_STOPPING_STATE_TIMEOUT_SECONDS = GOBBLIN_CLUSTER_PREFIX + "job.stoppingStateTimeoutSeconds";
  public static final long DEFAULT_HELIX_JOB_STOPPING_STATE_TIMEOUT_SECONDS = 300;
+  public static final String CONTAINER_HEALTH_METRICS_SERVICE_ENABLED = GOBBLIN_CLUSTER_PREFIX + "container.health.metrics.service.enabled" ;
 }


no default?

Added a default.

shirshanka · 2019-09-09T22:13:43Z

gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinTaskRunner.java

@@ -196,6 +196,11 @@ public GobblinTaskRunner(String applicationName,
    this.services.addAll(suite.getServices());

    this.services.addAll(getServices());
+
+    if (ConfigUtils.getBoolean(this.config, GobblinClusterConfigurationKeys.CONTAINER_HEALTH_METRICS_SERVICE_ENABLED, false)) {


Create a single static constant for the default value for this config, so you don't have to say false in two places?

shirshanka · 2019-09-09T22:29:24Z

gobblin-yarn/src/main/java/org/apache/gobblin/yarn/YarnHelixUtils.java

@@ -133,4 +131,8 @@ public static void addFileAsLocalResource(FileSystem fs, Path destFilePath, Loca

    return environmentVariableMap;
  }
+


javadoc missing.

Added javadoc.

shirshanka · 2019-09-09T22:29:33Z

gobblin-yarn/src/main/java/org/apache/gobblin/yarn/YarnHelixUtils.java

@@ -133,4 +131,8 @@ public static void addFileAsLocalResource(FileSystem fs, Path destFilePath, Loca

    return environmentVariableMap;
  }
+
+  public static String getContainerNum(String containerId) {
+    return "container-" + containerId.substring(containerId.lastIndexOf("_") + 1);


why are we doing + 1?

We want the substring starting from the char immediately following the last "_". e.g. if containerId = "container_e94_1567552810874_2132400_01_000001", we want to return
"container-000001". Added javadoc to make the behavior clear.

shirshanka · 2019-09-09T22:29:46Z

gobblin-cluster/src/test/java/org/apache/gobblin/cluster/ContainerHealthMetricsServiceTest.java

+    long processCpuTime2 = service.processCpuTime.get();
+    Assert.assertTrue(processCpuTime1 < processCpuTime2);
+  }
+}


missing newline at end of file.

Added newline.

shirshanka

LGTM!
Why is Travis unhappy?

…getServices().

autumnust

Minor comments. LGTM.

autumnust · 2019-09-16T16:43:19Z

gobblin-cluster/src/main/java/org/apache/gobblin/cluster/ContainerHealthMetricsService.java

+
+/**
+ * A utility class that periodically emits system level metrics that report the health of the container.
+ * Reported metrics include CPU/Memory usage of the JVM, system load, file descriptors used etc.


Which parameters are concerning with file descriptors?

Fixed the javadoc.

… in cluster mode Closes apache#2729 from sv2000/metrics

sv2000 added 2 commits September 9, 2019 09:40

GOBBLIN-875: Emit container health metrics when running in cluster mode.

723e5dd

GOBBLIN-875: Emit container health metrics when running in cluster mode.

a3f17cd

autumnust requested changes Sep 9, 2019

View reviewed changes

shirshanka suggested changes Sep 9, 2019

View reviewed changes

sv2000 added 2 commits September 9, 2019 17:21

GOBBLIN-875: Address reviewer comments.

2a0fa4d

GOBBLIN-875: Minor changes to ContainerHealthMetricsServiceTest

ab96b76

shirshanka approved these changes Sep 10, 2019

View reviewed changes

sv2000 added 2 commits September 9, 2019 20:18

GOBBLIN-685: Change to test case to address travis failure.

8cb863b

GOBBLIN-875: Move ContainerHealthMetricsService to GobblinTaskRunner#…

ed020c1

…getServices().

autumnust approved these changes Sep 16, 2019

View reviewed changes

GOBBLIN-875: Fixed the javadoc.

621c530

asfgit closed this in 9aa9e6e Sep 16, 2019

Will-Lo pushed a commit to Will-Lo/incubator-gobblin that referenced this pull request Sep 18, 2019

[GOBBLIN-875][GOBBLIN-685] Emit container health metrics when running…

4923dc3

… in cluster mode Closes apache#2729 from sv2000/metrics

jhsenjaliya pushed a commit to jhsenjaliya/incubator-gobblin that referenced this pull request Apr 26, 2020

[GOBBLIN-875][GOBBLIN-685] Emit container health metrics when running…

7e91bc0

… in cluster mode Closes apache#2729 from sv2000/metrics

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GOBBLIN-875: Emit container health metrics when running in cluster mode #2729

GOBBLIN-875: Emit container health metrics when running in cluster mode #2729

sv2000 commented Sep 9, 2019

autumnust Sep 9, 2019

sv2000 Sep 10, 2019

autumnust Sep 9, 2019

sv2000 Sep 10, 2019

autumnust Sep 10, 2019

codecov-io commented Sep 9, 2019 •

edited

Loading

shirshanka left a comment

shirshanka Sep 9, 2019

sv2000 Sep 10, 2019

shirshanka Sep 9, 2019

sv2000 Sep 10, 2019

shirshanka Sep 9, 2019

sv2000 Sep 10, 2019

shirshanka Sep 9, 2019

sv2000 Sep 10, 2019

shirshanka Sep 9, 2019

sv2000 Sep 10, 2019

shirshanka left a comment

autumnust left a comment

autumnust Sep 16, 2019

sv2000 Sep 16, 2019

		@@ -133,4 +131,8 @@ public static void addFileAsLocalResource(FileSystem fs, Path destFilePath, Loca

		return environmentVariableMap;
		}

GOBBLIN-875: Emit container health metrics when running in cluster mode #2729

GOBBLIN-875: Emit container health metrics when running in cluster mode #2729

Conversation

sv2000 commented Sep 9, 2019

JIRA

Description

Tests

Commits

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Sep 9, 2019 • edited Loading

Codecov Report

shirshanka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shirshanka left a comment

Choose a reason for hiding this comment

autumnust left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Sep 9, 2019 •

edited

Loading