-
Notifications
You must be signed in to change notification settings - Fork 859
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Sending empty parameter hashmap in case of retried execution #627
base: tuning_20190221
Are you sure you want to change the base?
Conversation
@@ -142,37 +142,40 @@ insert into yarn_app_heuristic_result_details (yarn_app_heuristic_result_id,name | |||
(137594640,'Number of tasks','20','NULL'); | |||
|
|||
INSERT INTO flow_definition(id, flow_def_id, flow_def_url) VALUES | |||
(10003,'https://ltx1-holdemaz01.grid.linkedin.com:8443/manager?project=AzkabanHelloPigTest&flow=countByCountryFlow','https://ltx1-holdemaz01.grid.linkedin.com:8443/manager?project=AzkabanHelloPigTest&flow=countByCountryFlow'); | |||
(10003,'https://ltx1-holdemaz01.grid.linkedin.com:8443/manager?project=AzkabanHelloPigTest&flow=countByCountryFlow','https://ltx1-holdemaz01.grid.linkedin.com:8443/manager?project=AzkabanHelloPigTest&flow=countByCountryFlow'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kindly remove internal Url links.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These lines are already there. I have not modified them except for ';' changed to ',' as I wanted to another line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, but you can help with correcting this and if need can ask for the help of the author of the respective part.
applyPenalty(tuningInput.getJobExecId()); | ||
jobSuggestedParamSet = getBestParamSet(tuningInput.getJobDefId()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will there be no parameters suggested in case of retry?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. It sends empty hashmap so that default parameters can be applied for retried execution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So was getting bestParameters
and sending them was a BUG which you fixed in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes because after getting these parameters dr. elephant is adding the retried job execution to TuningJobExecutionParamSet against these parameters which is not correct as this is a dummy call. Retried execution always run with default parameters. It ignores the parameters sent by dr. elephant.
Therefore this step was unnecessary step and next step was not correct.
if (tuningInput.getRetry()) { | ||
logger.info(" Retry "); | ||
logger.info(" Retried job execution " + tuningInput.getJobExecId()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this method called when execution is re-tried or is about to retry?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
About to retry. Will correct in next commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
@dushyantk1509 Can you mention the problem statement in the Description section of the PR as it will provide more context about the PR. Also the doc link you mentioned above is not accessible for me using my external email-id, can you have a look so that it would be available to all. |
Now you won't need docs. |
* Integrating Spark and MR exception fingerprinting so results for both will be visible on Dr.Elephant's UI * Changing OracleJDK to OpenJDK as travisCI is not supporting OracleJDK8 * Adding IDENTITY generation strategy for ID * Added unit tests for Exception Fingerprinting * Adding test files * Addressing the review comments
When a job is retried, it makes a dummy getCurrentRunParameter call to dr. elephant. In this way tuneIN knows that there was a failure with earlier parameters and penalize the parameters accordingly. In current implementation, best parameters are returned as an output of this dummy call even though we don’t apply these parameters and a new entry is made in tuningJobExecutionParamSet which is not correct.
In this change, empty parameter hash map will return as an output of retried dummy getCurrentRunParameter call and make entry in tuningJobExecutionParamSet only for non-tried executions.