You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@gobblin.apache.org by GitBox <gi...@apache.org> on 2020/07/21 02:28:19 UTC

[GitHub] [incubator-gobblin] arjun4084346 opened a new pull request #3065: [GOBBLIN-1217] start metrics reporting with a few map-reduce properties

arjun4084346 opened a new pull request #3065:
URL: https://github.com/apache/incubator-gobblin/pull/3065

Dear Gobblin maintainers,

Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below!
@sv2000 @jack-moseley please review

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

### Description
- [x] Here are some details about my PR, including screenshots (if applicable):
Add a few map/reduce task related configs in building metric reporter. One of the use case can be that each mapper/reducer's metrics can be analyzed separately

### Tests
- [x] My PR adds the following unit tests __OR__ does not need testing for this extremely good reason:
trivial changes

### Commits
- [x] 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](http://chris.beams.io/posts/git-commit/)":
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"

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

[GitHub] [incubator-gobblin] sv2000 commented on a change in pull request #3065: [GOBBLIN-1217] start metrics reporting with a few map-reduce properties

Posted by GitBox <gi...@apache.org>.

sv2000 commented on a change in pull request #3065:
URL: https://github.com/apache/incubator-gobblin/pull/3065#discussion_r458310351



##########
File path: gobblin-runtime/src/main/java/org/apache/gobblin/runtime/mapreduce/MRJobLauncher.java
##########
@@ -771,6 +780,20 @@ protected void setup(Context context) {
         gobblinJobState.setProp(entry.getKey(), entry.getValue().unwrapped().toString());
       }
 
+      // add some more MR task related configs
+
+      String[] tokens = taskAttemptId.toString().split("_");
+      TaskType taskType = taskAttemptId.getTaskType();
+      gobblinJobState.setProp(MR_TYPE_KEY, taskType.name());
+
+      if (taskType.equals(TaskType.MAP)) {
+        gobblinJobState.setProp(MAPPER_TASK_NUM_KEY, tokens[4]);

Review comment:
       Add a check for the length of the "tokens" array or an assertion that tokens should be of length 6. Further, you may want to access the map/reduce task num as tokens[tokens.length - 1].  




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

[GitHub] [incubator-gobblin] asfgit closed pull request #3065: [GOBBLIN-1217] start metrics reporting with a few map-reduce properties

Posted by GitBox <gi...@apache.org>.

asfgit closed pull request #3065:
URL: https://github.com/apache/incubator-gobblin/pull/3065


   


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

[GitHub] [incubator-gobblin] codecov-commenter commented on pull request #3065: [GOBBLIN-1217] start metrics reporting with a few map-reduce properties

Posted by GitBox <gi...@apache.org>.

codecov-commenter commented on pull request #3065:
URL: https://github.com/apache/incubator-gobblin/pull/3065#issuecomment-661670766


   # [Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/3065?src=pr&el=h1) Report
   > Merging [#3065](https://codecov.io/gh/apache/incubator-gobblin/pull/3065?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-gobblin/commit/97774251028668486ece928e3290a5d298bf57b6&el=desc) will **increase** coverage by `0.00%`.
   > The diff coverage is `0.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-gobblin/pull/3065/graphs/tree.svg?width=650&height=150&src=pr&token=4MgURJ0bGc)](https://codecov.io/gh/apache/incubator-gobblin/pull/3065?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff            @@
   ##             master    #3065   +/-   ##
   =========================================
     Coverage     45.86%   45.87%           
     Complexity     9423     9423           
   =========================================
     Files          1963     1963           
     Lines         74742    74752   +10     
     Branches       8310     8314    +4     
   =========================================
   + Hits          34282    34289    +7     
     Misses        37231    37231           
   - Partials       3229     3232    +3     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-gobblin/pull/3065?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...pache/gobblin/configuration/ConfigurationKeys.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3065/diff?src=pr&el=tree#diff-Z29iYmxpbi1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vY29uZmlndXJhdGlvbi9Db25maWd1cmF0aW9uS2V5cy5qYXZh) | `0.00% <ø> (ø)` | `0.00 <0.00> (ø)` | |
   | [...ava/org/apache/gobblin/metrics/GobblinMetrics.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3065/diff?src=pr&el=tree#diff-Z29iYmxpbi1tZXRyaWNzLWxpYnMvZ29iYmxpbi1tZXRyaWNzL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL21ldHJpY3MvR29iYmxpbk1ldHJpY3MuamF2YQ==) | `31.21% <0.00%> (-0.89%)` | `30.00 <0.00> (ø)` | |
   | [...n/java/org/apache/gobblin/yarn/YarnHelixUtils.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3065/diff?src=pr&el=tree#diff-Z29iYmxpbi15YXJuL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3lhcm4vWWFybkhlbGl4VXRpbHMuamF2YQ==) | `18.42% <ø> (ø)` | `4.00 <0.00> (ø)` | |
   | [.../modules/scheduler/GobblinServiceJobScheduler.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3065/diff?src=pr&el=tree#diff-Z29iYmxpbi1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9kdWxlcy9zY2hlZHVsZXIvR29iYmxpblNlcnZpY2VKb2JTY2hlZHVsZXIuamF2YQ==) | `57.14% <0.00%> (-2.12%)` | `24.00% <0.00%> (-2.00%)` | |
   | [.../org/apache/gobblin/cluster/GobblinTaskRunner.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3065/diff?src=pr&el=tree#diff-Z29iYmxpbi1jbHVzdGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NsdXN0ZXIvR29iYmxpblRhc2tSdW5uZXIuamF2YQ==) | `63.46% <0.00%> (-0.33%)` | `35.00% <0.00%> (ø%)` | |
   | [.../apache/gobblin/runtime/api/JobExecutionState.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3065/diff?src=pr&el=tree#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvYXBpL0pvYkV4ZWN1dGlvblN0YXRlLmphdmE=) | `80.37% <0.00%> (+0.93%)` | `24.00% <0.00%> (ø%)` | |
   | [...java/org/apache/gobblin/metrics/MetricContext.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3065/diff?src=pr&el=tree#diff-Z29iYmxpbi1tZXRyaWNzLWxpYnMvZ29iYmxpbi1tZXRyaWNzLWJhc2Uvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vbWV0cmljcy9NZXRyaWNDb250ZXh0LmphdmE=) | `80.91% <0.00%> (+1.52%)` | `50.00% <0.00%> (ø%)` | |
   | [...lin/restli/throttling/ZookeeperLeaderElection.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3065/diff?src=pr&el=tree#diff-Z29iYmxpbi1yZXN0bGkvZ29iYmxpbi10aHJvdHRsaW5nLXNlcnZpY2UvZ29iYmxpbi10aHJvdHRsaW5nLXNlcnZpY2Utc2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3Jlc3RsaS90aHJvdHRsaW5nL1pvb2tlZXBlckxlYWRlckVsZWN0aW9uLmphdmE=) | `72.22% <0.00%> (+2.22%)` | `13.00% <0.00%> (ø%)` | |
   | [...in/java/org/apache/gobblin/cluster/HelixUtils.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3065/diff?src=pr&el=tree#diff-Z29iYmxpbi1jbHVzdGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NsdXN0ZXIvSGVsaXhVdGlscy5qYXZh) | `37.50% <0.00%> (+5.83%)` | `14.00% <0.00%> (+2.00%)` | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/3065?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/3065?src=pr&el=footer). Last update [9777425...0a9a105](https://codecov.io/gh/apache/incubator-gobblin/pull/3065?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


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

[GitHub] [incubator-gobblin] sv2000 commented on a change in pull request #3065: [GOBBLIN-1217] start metrics reporting with a few map-reduce properties

Posted by GitBox <gi...@apache.org>.

sv2000 commented on a change in pull request #3065:
URL: https://github.com/apache/incubator-gobblin/pull/3065#discussion_r458190950



##########
File path: gobblin-metrics-libs/gobblin-metrics/src/main/java/org/apache/gobblin/metrics/GobblinMetrics.java
##########
@@ -367,6 +374,20 @@ public void startMetricReportingWithFileSuffix(State state, String metricsFileSu
       oldMetricsFileSuffix += "." + metricsFileSuffix;
     }
     metricsReportingProps.setProperty(ConfigurationKeys.METRICS_FILE_SUFFIX, oldMetricsFileSuffix);
+
+    if (!Strings.isNullOrEmpty(metricsFileSuffix)) {

Review comment:
       I think GobblinMetrics should be agnostic to the execution mode of the job, and shouldn't have to worry about extracting mapper/reducer num from the task id. This block can be moved inside MRJobLauncherClass and the configurations such as MAPPER_TASK_NUM_KEY can be set there and added into the state object. 

##########
File path: gobblin-api/src/main/java/org/apache/gobblin/configuration/ConfigurationKeys.java
##########
@@ -708,6 +708,11 @@
   public static final String DEFAULT_METRICS_REPORTING_FILE_ENABLED = Boolean.toString(false);
   public static final String METRICS_LOG_DIR_KEY = METRICS_CONFIGURATIONS_PREFIX + "log.dir";
   public static final String METRICS_FILE_SUFFIX = METRICS_CONFIGURATIONS_PREFIX + "reporting.file.suffix";
+  public static final String MR_TYPE_KEY = METRICS_CONFIGURATIONS_PREFIX + "mr.type";

Review comment:
       Since these configuration keys are dynamically set and not user provided, its better to define them inside the MRJobLauncher class.




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

[GitHub] [incubator-gobblin] jack-moseley commented on a change in pull request #3065: [GOBBLIN-1217] start metrics reporting with a few map-reduce properties

Posted by GitBox <gi...@apache.org>.

jack-moseley commented on a change in pull request #3065:
URL: https://github.com/apache/incubator-gobblin/pull/3065#discussion_r457895558



##########
File path: gobblin-metrics-libs/gobblin-metrics/src/main/java/org/apache/gobblin/metrics/GobblinMetrics.java
##########
@@ -367,6 +374,20 @@ public void startMetricReportingWithFileSuffix(State state, String metricsFileSu
       oldMetricsFileSuffix += "." + metricsFileSuffix;
     }
     metricsReportingProps.setProperty(ConfigurationKeys.METRICS_FILE_SUFFIX, oldMetricsFileSuffix);
+
+    if (!Strings.isNullOrEmpty(metricsFileSuffix)) {
+      String[] tokens = metricsFileSuffix.split("_");

Review comment:
       I just googled and found that there are mapreduce classes for these such as `TaskAttemptID`. Would it be possible to make use of these to get the properties through APIs instead of parsing the string manually?




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

[GitHub] [incubator-gobblin] arjun4084346 commented on a change in pull request #3065: [GOBBLIN-1217] start metrics reporting with a few map-reduce properties

Posted by GitBox <gi...@apache.org>.

arjun4084346 commented on a change in pull request #3065:
URL: https://github.com/apache/incubator-gobblin/pull/3065#discussion_r458302607



##########
File path: gobblin-metrics-libs/gobblin-metrics/src/main/java/org/apache/gobblin/metrics/GobblinMetrics.java
##########
@@ -367,6 +374,20 @@ public void startMetricReportingWithFileSuffix(State state, String metricsFileSu
       oldMetricsFileSuffix += "." + metricsFileSuffix;
     }
     metricsReportingProps.setProperty(ConfigurationKeys.METRICS_FILE_SUFFIX, oldMetricsFileSuffix);
+
+    if (!Strings.isNullOrEmpty(metricsFileSuffix)) {
+      String[] tokens = metricsFileSuffix.split("_");

Review comment:
       I could not find any API which gives task number and attempt. getTaskAttemptID().getJobID(), getTaskAttemptID().getTaskID(), getTaskAttemptID().getId() did not quite give what I am trying to extract from taskAttemptID.
   I'll use context.getTaskAttemptID().getTaskType()




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