Fixes MapReduce aggregator and heuristic to correctly handle task data when sampling is enabled #222
Conversation
…a when sampling is enabled (linkedin#33) * Fixes MapReduce aggregator and heuristic to correctly handle task data when sampling is enabled. * Adds a comment
|
Please take a look. |
| @@ -106,6 +106,9 @@ private void compute(MapReduceTaskData[] taskDatas, long containerSize, long ide | |||
| } | |||
|
|
|||
| for (MapReduceTaskData taskData: taskDatas) { | |||
| if (!taskData.isTimeAndCounterDataPresent()) { | |||
There was a problem hiding this comment.
Isn't this always true ? Can you elaborate on what exactly is the issue you are trying to fix ? I remember you mentioning something in the meetup but dont recollect exactly what the issue was.
There was a problem hiding this comment.
No, it's not always true. If sampling is enabled, then it's only going to be true for sampled tasks. In MapReduceFetcherHadoop2 class, MapReduceTaskData instance is created for all the tasks, but time and counter data is only stored for sampled tasks. Please look at the logic here.
The unit test in this change will fail because NullPointerException without this check.
| import java.util.Properties; | ||
| import java.util.concurrent.TimeUnit; | ||
|
|
||
| import com.linkedin.drelephant.analysis.Heuristic; | ||
| import com.linkedin.drelephant.analysis.HeuristicResult; | ||
| import com.linkedin.drelephant.analysis.Severity; | ||
|
|
||
|
|
||
| public class JobQueueLimitHeuristic implements Heuristic<MapReduceApplicationData> { |
There was a problem hiding this comment.
@akshayrai Is this heuristic used at linkedin ? I dont see it enabled.
There was a problem hiding this comment.
No. We don't use this. As far as I remember, this heuristic was created to warn jobs which were hitting close to the default queue time out limit of 15mins.
| @@ -106,6 +106,9 @@ private void compute(MapReduceTaskData[] taskDatas, long containerSize, long ide | |||
| } | |||
|
|
|||
| for (MapReduceTaskData taskData: taskDatas) { | |||
| if (!taskData.isTimeAndCounterDataPresent()) { | |||
There was a problem hiding this comment.
Can you add a comment to the function saying that the aggregatemetrics are not expected to be accurate when sampling is enabled so that it's clear ?
There was a problem hiding this comment.
Thanks @shankar37 for the review.
Added a comment. PTAL.
|
+1 |
…a when sampling is enabled (linkedin#222)
Also updates all the heuristic unit tests to take non-sampled tasks into account.