Pig Job Auto Tuning Integration #290

mkumar1984 · 2018-05-10T14:37:44Z

This change is for integrating Pig job with auto tuning framework (https://github.com/linkedin/dr-elephant/wiki/Auto-Tuning). Auto Tuning framework provides a way to automatically tune hadoop jobs. Currently it supports Pig Job for resource usage optimization. Integration is pretty easy. It requires calling getCurrentRunParameters API before running any job. This API responds with a parameter suggestion. These parameters should be used for the configuration of the job.

Design

Currently configurations are injected using HadoopConfigurationInjector. We have written a new HadoopTuningConfigurationInjector. This new injector will inject default configuration first and then call getCurrentRunParameter API to get current run of parameters from AutoTuning framework and inject those parameters in configuration of the job.
Failure Handling
-- API down: In case API is down, fallback method would be to let job use default configuration.
-- Other failed reason: In case there is other failures, we will identify if this is because of auto tuning by analyzing log and then retry getCurrentRunParameter API with isRetry flag enabled. In this case getCurrentRunParameter returns best parameters from already tried parameters.
We will create a new wrapper HadoopTuneInSecurePigWrapper, similar to HadoopSecurePigWrapper which will have this new flow of injecting parameter, handling failures and calling status API. And based on flag auto_tuning_enabled (default false), we can choose which wrapper to use. (HadoopPigJob) Another approach could be to use auto_tuning_enabled flag to decide on flow and don’t create HadoopTuneInSecurePigWrapper.
We will avoid doing configuration override in the script and will ask users to not keep any configuration inside the script for enabling AutoTuning.

Manual Test Cases
AzkabanPigJobTypeTestCases.xlsx

xkrogen

@mkumar1984 , thanks for the contribution!

I have two overall comments:

There should be much more code reuse, and more use of existing constants rather than redefining them.
I think the tests seem a little bit lacking. I would love to see something that mocks out the HTTP request to verify expected input, return expected output, and see that configs are updated accordingly.

xkrogen · 2018-05-17T15:53:39Z

plugins/jobtype/jobtypes/pig-0.12.0/plugin.properties

+#pig.enable.tuning=false
+#tuning.api.end.point=http://hostname:8080/rest/getCurrentRunParameters
+#auto.tuning.job.type=PIG


What's going on here? Why are we adding commented-out properties?

These are just kept for references, if user want to enable tuning they can do that. I have un-commented 2 of them.

xkrogen · 2018-05-17T15:55:02Z

plugins/jobtype/src/azkaban/jobtype/HadoopPigJob.java

+  public static final String JOB_ENABLE_TUNING = "job.enable.tuning";
+
+  //Internal flag for seeing whether this was a retry of the job after failure because of auto tuning
+  public static final String AUTO_TUNING_RETRY = "auto.tuning.retry";


Is there no cleaner way to pass this around than as a job prop?

Also, if this is an internal flag, it should not be public.

This variable was not required here.

All other parameters are passed through job props, so thought of using same.

xkrogen · 2018-05-17T15:56:50Z

plugins/jobtype/src/azkaban/jobtype/HadoopPigJob.java

+    }
+    if(enableTuning)
+    {
+      HADOOP_SECURE_PIG_WRAPPER = HadoopTuningSecurePigWrapper.class.getName();


We should not be modifying a static variable here. IIUC this could affect other running jobs.

HADOOP_SECURE_PIG_WRAPPER should really be a constant (final), and there should be a new field like private final String securePigWrapper which contains the wrapper to use for this specific instance.

Yes, you are right here. Great catch. Done.

xkrogen · 2018-05-17T15:57:36Z

plugins/jobtype/src/azkaban/jobtype/HadoopPigJob.java

-    HADOOP_SECURE_PIG_WRAPPER = HadoopSecurePigWrapper.class.getName();
+    if (jobProps.containsKey(JOB_ENABLE_TUNING) && jobProps.containsKey(PIG_ENABLE_TUNING)) {
+      enableTuning = jobProps.getBoolean(JOB_ENABLE_TUNING) && jobProps.getBoolean(PIG_ENABLE_TUNING);
+    }else


Style does not match convention. }else -> } else {, if( -> if (, and opening curly brace should appear on the same line as the if.

xkrogen · 2018-05-17T15:57:52Z

plugins/jobtype/src/azkaban/jobtype/HadoopPigJob.java

@@ -248,6 +269,7 @@ protected String getMainArguments() {
    classPath.add(HadoopConfigurationInjector.getPath(getJobProps(),
        getWorkingDirectory()));

+


remove unnecessary whitespace changes

xkrogen · 2018-05-17T16:42:52Z

plugins/jobtype/test/azkaban/jobtype/TestHadoopTuningConfigurationInjector.java

+    props.put(HadoopConfigurationInjector.INJECT_PREFIX + "mapreduce.reduce.memory.mb", "2048");
+    props.put(HadoopConfigurationInjector.INJECT_PREFIX + "mapreduce.map.memory.mb", "2048");
+
+    props.put(CommonJobProperties.EXEC_ID, CommonJobProperties.EXEC_ID);


Could use a loop here to eliminate a lot of code

Probably create an array of all of these props you want to add, iterate through that array here and put them into props, then iterate through it again at the end and verify that it hasn't changed

xkrogen · 2018-05-17T16:44:11Z

plugins/jobtype/test/azkaban/jobtype/TestHadoopTuningConfigurationInjector.java

+public class TestHadoopTuningConfigurationInjector {
+
+  @Test
+  public void testTuningErrorHandler() throws IOException {


Can you explain how the tuning error handler is being used here? What is actually being tested?

Typo in method name. Changed the name.

xkrogen · 2018-05-17T16:44:48Z

plugins/jobtype/test/azkaban/jobtype/TestTuningErrorHandler.java

@@ -0,0 +1,38 @@
+package azkaban.jobtype;


xkrogen · 2018-05-17T16:45:09Z

plugins/jobtype/test/azkaban/jobtype/TestTuningParameterUtils.java

@@ -0,0 +1,56 @@
+package azkaban.jobtype;


xkrogen · 2018-05-17T16:45:24Z

plugins/jobtype/test/azkaban/jobtype/TestTuningParameterUtils.java

+  @Test
+  public void testGetHadoopProperties() throws IOException {
+    Props props = new Props();
+    props.put(HadoopConfigurationInjector.INJECT_PREFIX + "mapreduce.task.io.sort.mb", "100");


use Hadoop constants here

inramana

@xkrogen covered my biggest concerns- Those of duplicate code in the HadoopConfigurationInjector and HadoopSecurePigWrapper. Also I do not think this is following the azkaban style guide in a number of places. Please refer to https://github.com/azkaban/azkaban/blob/master/CONTRIBUTING.md

inramana · 2018-05-17T19:32:59Z

plugins/jobtype/src/azkaban/jobtype/HadoopTuningConfigurationInjector.java

+ * using injectResources() so that they are included in any Configuration
+ * constructed.
+ */
+public class HadoopTuningConfigurationInjector {


+1
It seems like almost all of the code is duplicated apart from a few lines.

mkumar1984 · 2018-05-19T20:45:13Z

Have taken care of all of the reviews except writing test using mock API call. This will take some time. Need to explore a bit.

xkrogen

Looking really good after the last set of changes; thanks for incorporating all of my feedback! This time around is mostly just small style type nits. The only other outstanding thing is that I would like to see is a more full test with some HTTP endpoint mocking, which I believe you said you're working on already.

xkrogen · 2018-05-21T15:48:27Z

plugins/jobtype/src/azkaban/jobtype/HadoopConfigurationInjector.java

@@ -50,6 +50,7 @@

  public static final String WORKFLOW_ID_SEPERATOR = "$";
  private static final String WORKFLOW_ID_CONFIG = "yarn.workflow.id";
+


Seems like this and a few other style changes in this class are not related to this change. Typically I discourage style changes which are not a part of the core of the change. Not sure if this is also the recommended approach for Azkaban code.

Not sure how this white space gets added. Removed for this case.

xkrogen · 2018-05-21T15:52:50Z

plugins/jobtype/src/azkaban/jobtype/HadoopTuningConfigurationInjector.java

+ * using injectResources() so that they are included in any Configuration
+ * constructed.
+ */
+public class HadoopTuningConfigurationInjector {


Nice! Looks great

xkrogen · 2018-05-21T15:54:06Z

plugins/jobtype/src/azkaban/jobtype/HadoopTuningSecurePigWrapper.java

+      jobTryCount++;
+      firstTry = false;
+      props = Props.clone(initialJobprops);
+      props.put(TuningCommonConstants.AUTO_TUNING_RETRY, retry + "");


String.valueOf() instead of + "" ?

xkrogen · 2018-05-21T15:55:06Z

plugins/jobtype/src/azkaban/jobtype/HadoopTuningSecurePigWrapper.java

+
+  /**
+   * In case if job is failed because of auto tuning parameters, it will be retried with the best parameters
+   * we have seen so far. Maximum number of try is 2.


"Maximum number of try is 2" => "Maximum number of tries is 2 by default"

xkrogen · 2018-05-21T15:58:39Z

plugins/jobtype/src/azkaban/jobtype/HadoopTuningSecurePigWrapper.java

+          isTuningError = true;
+        }
+      }
+      reader.close();


This should be in a finally clause. Ideally, use a try-with-resources statement.

Done. Used a try-with-resources statement. Thanks for the suggestion. Didn't know about this earlier.

xkrogen · 2018-05-21T16:07:57Z

plugins/jobtype/src/azkaban/jobtype/tuning/TuningParameterUtils.java

+        retryCount++;
+        log.info("Calling get current run parameters. Try count " + retryCount);
+        params = getCurrentRunParameters(props);
+        noException = true;


I think it would be cleaner to do away with the noException variable and simply replace this line with break

xkrogen · 2018-05-21T16:08:59Z

plugins/jobtype/src/azkaban/jobtype/tuning/TuningParameterUtils.java

+  }
+
+  /**
+   *This method is for getting current run parameters from Dr Elephant


Spacing in this line of Javadoc, and in general the Javadocs in this class could be filled out a bit more completely (e.g. there is no description of the arguments)

xkrogen · 2018-05-21T16:09:27Z

plugins/jobtype/src/azkaban/jobtype/tuning/TuningParameterUtils.java

+    CloseableHttpClient httpClient = HttpClients.createDefault();
+
+    HttpResponse response = null;
+    String endPoint = String.format("%s", props.get(TuningCommonConstants.TUNING_API_END_POINT));


What is being achieved with this line?

Earlier was using multiple string concatenation. Removed the string.format. Directly using property.

xkrogen · 2018-05-21T16:12:00Z

plugins/jobtype/test/azkaban/jobtype/TestHadoopTuningConfigurationInjector.java

+    userProps.put(HadoopConfigurationInjector.INJECT_PREFIX + MRJobConfig.REDUCE_MEMORY_MB, "2048");
+    userProps.put(HadoopConfigurationInjector.INJECT_PREFIX + MRJobConfig.MAP_MEMORY_MB, "2048");
+
+    azkabanProps.put(CommonJobProperties.EXEC_ID, CommonJobProperties.EXEC_ID);


I think I mentioned this in the last review, but don't see my comment / a response - can we set all of these properties via a loop?

xkrogen · 2018-05-21T16:14:44Z

plugins/jobtype/src/azkaban/jobtype/tuning/TuningErrorHandler.java

+ * This class is responsible for finding whether failure is because of tuning parameters.
+ * This try to search predefined patterns in the log.
+ */
+public class TuningErrorHandler {


This class doesn't actually handle tuning errors, just detects them. Maybe it should be called TuningErrorDetector ?

Changed to TuningErrorDetector.

HappyRay · 2018-05-21T18:18:38Z

@xkrogen and @inramana
Thanks for the reviews.

In case you are not aware yet, I wrote a tip for how to update copyright automatically. see https://github.com/azkaban/azkaban/wiki/Developer-Tools-and-Tips#use-intellij-to-create-and-update-copyright-automatically

xkrogen · 2018-05-21T22:58:32Z

Just reduced verbosity: https://www.javaworld.com/article/2074080/core-java/jdk-7--the-diamond-operator.html

…

On Mon, May 21, 2018 at 1:42 PM mkumar1984 ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In plugins/jobtype/src/azkaban/jobtype/tuning/TuningErrorHandler.java <#290 (comment)> : > +import java.util.ArrayList; +import java.util.List; +import java.util.regex.Matcher; +import java.util.regex.Pattern; + +import org.apache.log4j.Logger; + + +/** + * This class is responsible for finding whether failure is because of tuning parameters. + * This try to search predefined patterns in the log. + */ +public class TuningErrorHandler { + private Logger log = Logger.getRootLogger(); + + private static List<Pattern> errorPatterns = new ArrayList<Pattern>(); Done. BTW what's the difference between two. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#290 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGRBoQZkybwMU-cuhH8pWMQA9loQRqx4ks5t0yaugaJpZM4T5_09> .

…rameters

mkumar1984 · 2018-05-25T10:23:34Z

Added a test case for TestTuningParameterUtils.java to mock the API and test updateAutoTuningParameters. This should handle all the review comments. Let me know if there is any other comments.

xkrogen

This is looking really good! Thanks for addressing our concerns thoroughly about the new tests. I left a few more comments, mostly minor. Also, the new mock test looks great, but can we also add one for the failure case (return a 500, 404, something like that)?

xkrogen · 2018-05-25T16:02:56Z

plugins/jobtype/src/azkaban/jobtype/HadoopTuningSecurePigWrapper.java

+
+  private static File pigLogFile;
+
+  private static Props props;


Whoops, sounds good.

xkrogen · 2018-05-25T16:03:49Z

plugins/jobtype/src/azkaban/jobtype/HadoopTuningSecurePigWrapper.java

+        if (HadoopSecureWrapperUtils.shouldProxy(jobProps)) {
+          String tokenFile = System.getenv(HADOOP_TOKEN_FILE_LOCATION);
+          UserGroupInformation proxyUser = HadoopSecureWrapperUtils.setupProxyUser(jobProps, tokenFile, logger);
+          proxyUser.doAs(new PrivilegedExceptionAction<Void>() {


In that case, this could be greatly simplified like proxyUser.doAs(() -> runPigJob(args));

xkrogen · 2018-05-25T16:06:25Z

plugins/jobtype/test/azkaban/jobtype/TestTuningParameterUtils.java

+    props.put(HadoopConfigurationInjector.INJECT_PREFIX + MRJobConfig.MAP_MEMORY_MB, "2048");
+    props.put(TuningCommonConstants.TUNING_API_END_POINT, "dummy_api_end_point");
+
+    Props passingProps=new Props(null, props);


space around =

xkrogen · 2018-05-25T16:06:44Z

plugins/jobtype/test/azkaban/jobtype/TestTuningParameterUtils.java

+    Assert.assertEquals(MRJobConfig.IO_SORT_FACTOR + " value not correct",
+        passingProps.getString(HadoopConfigurationInjector.INJECT_PREFIX + MRJobConfig.IO_SORT_FACTOR), "10");
+
+    passingProps=new Props(null, props);


space around =

xkrogen · 2018-05-25T16:06:55Z

plugins/jobtype/test/azkaban/jobtype/TestTuningParameterUtils.java

+        passingProps.getString(HadoopConfigurationInjector.INJECT_PREFIX + MRJobConfig.MAP_MEMORY_MB), "2048");
+    Assert.assertEquals(MRJobConfig.SHUFFLE_INPUT_BUFFER_PERCENT + " value not correct",
+        passingProps.getString(HadoopConfigurationInjector.INJECT_PREFIX + MRJobConfig.SHUFFLE_INPUT_BUFFER_PERCENT), "0.9");
+


unnecessary blank

xkrogen · 2018-05-25T16:12:58Z

plugins/jobtype/test/azkaban/jobtype/TestTuningParameterUtils.java

+
+    CloseableHttpClient httpClient = Mockito.mock(CloseableHttpClient.class);
+    PowerMockito.when(HttpClients.createDefault()).thenReturn(httpClient);
+    PowerMockito.when(EntityUtils.toString(Matchers.any(HttpEntity.class))).thenReturn(expectedJSON);


Rather than mocking out a static method, can you simply have your HTTP client return a StringEntity with a string defined by you? See http://grepcode.com/file/repo1.maven.org/maven2/org.apache.httpcomponents/httpcore/4.1/org/apache/http/entity/StringEntity.java Then EntityUtils should be able to stringify it properly.

xkrogen · 2018-05-25T16:15:00Z

plugins/jobtype/test/azkaban/jobtype/TestTuningParameterUtils.java

+    PowerMockito.mockStatic(EntityUtils.class);
+
+    CloseableHttpClient httpClient = Mockito.mock(CloseableHttpClient.class);
+    PowerMockito.when(HttpClients.createDefault()).thenReturn(httpClient);


Though PowerMockito's static mocking abilities are definitely powerful, it also relies on rewriting bytecode to redirect calls somewhat magically. My inclination is that this should only be used as a last resort, for example when dealing with library code that you cannot easily modify. I would prefer to see the updateAutoTuningParameters() method refactored slightly to allow for you to pass in your own HttpClient, avoiding this type of mocking. Let me know if you disagree.

Agree. Removing PowerMockito's usage. Changed TuningParameterUtils to have all methods as static.

xkrogen · 2018-05-29T16:16:44Z

LGTM, great work @mkumar1984 !

inramana · 2018-06-13T22:02:02Z

@mkumar1984 can you resolve conflicts?
I can merge this once you are done.

kunkun-tang · 2018-06-13T22:06:06Z

I believe the code has been moved to azkaban main repo.

Pig Job Auto Tuning Integration

a534689

inramana self-requested a review May 16, 2018 17:11

xkrogen suggested changes May 17, 2018

View reviewed changes

inramana suggested changes May 17, 2018

View reviewed changes

Addressing Review comment - 1

f2fdb88

xkrogen suggested changes May 21, 2018

View reviewed changes

Addressing review comments - 2

fb689f1

Add Test cases for mocking getCurrentRunParameter API and updating pa…

ec7ce85

…rameters

xkrogen suggested changes May 25, 2018

View reviewed changes

Adding more test cases and addressing review comments

29e81d8

inramana approved these changes Jun 13, 2018

View reviewed changes

		@@ -248,6 +269,7 @@ protected String getMainArguments() {
		classPath.add(HadoopConfigurationInjector.getPath(getJobProps(),
		getWorkingDirectory()));

		@@ -50,6 +50,7 @@

		public static final String WORKFLOW_ID_SEPERATOR = "$";
		private static final String WORKFLOW_ID_CONFIG = "yarn.workflow.id";

Pig Job Auto Tuning Integration #290

Are you sure you want to change the base?

Pig Job Auto Tuning Integration #290

Conversation

mkumar1984 commented May 10, 2018 • edited Loading

xkrogen 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

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

inramana left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkumar1984 commented May 19, 2018

xkrogen 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

mkumar1984 May 21, 2018 • edited Loading

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

HappyRay commented May 21, 2018

xkrogen commented May 21, 2018 via email

mkumar1984 commented May 25, 2018

xkrogen 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xkrogen commented May 29, 2018

inramana commented Jun 13, 2018

kunkun-tang commented Jun 13, 2018

mkumar1984 commented May 10, 2018 •

edited

Loading

mkumar1984 May 21, 2018 •

edited

Loading