MAPREDUCE-7369. Fixed MapReduce tasks timing out when spends more time on MultipleOutputs#close #4247
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…e on MultipleOutputs#close
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
iwasakims
reviewed
May 3, 2022
|
|
||
| <property> | ||
| <name>mapreduce.task.enable.ping-for-liveliness-check</name> | ||
| <value>true</value> |
Member
There was a problem hiding this comment.
The value must be same with the DEFAULT_MR_TASK_ENABLE_PING_FOR_LIVELINESS_CHECK.
Contributor
Author
There was a problem hiding this comment.
Thanks @iwasakims for review. I have updated the DEFAULT_MR_TASK_ENABLE_PING_FOR_LIVELINESS_CHECK to true.
Member
There was a problem hiding this comment.
When should we turn this off? If it should be always true, we don't need to make it configurable. We usually needs new configuration knob to add new feature disabled by default for compatibility and safety.
Contributor
Author
There was a problem hiding this comment.
Yeah I think this makes sense, I will remove configurable part.
Contributor
Author
There was a problem hiding this comment.
Hi @iwasakims. I saw the comment from @cnauroth on JIRA about his suggestion to put it behind the configuration and I think we can keep it configurable and let the end user decide on it. By default, lets keep it
mapreduce.task.enable.ping-for-liveliness-check : false which will keep the current behaviour intact
and for use cases that we saw in JIRA, user can set it true when required
Thoughts?
Member
There was a problem hiding this comment.
@ashutoshcipher Making this feature disabled by default sounds right direction as @cnauroth mentioned. While no unexpected side effect was observed in the test results of current patch, there should be additional test case covering the code path of new feature enabled.
Contributor
Author
There was a problem hiding this comment.
Thanks @iwasakims , let me make the required changes.
Contributor
Author
There was a problem hiding this comment.
@iwasakims @cnauroth - I have made the required changes, Can you please review the PR? Thanks
|
🎊 +1 overall
This message was automatically generated. |
Member
|
It would be nice if we can add a test case reproducing the issue covered by this fix. |
|
🎊 +1 overall
This message was automatically generated. |
… ping check feature
Contributor
Author
Reproducing this issue seems a little tricky, But I have added a test case for this feature. Thanks. |
iwasakims
approved these changes
Jun 19, 2022
Member
iwasakims
left a comment
iwasakims
left a comment
There was a problem hiding this comment.
Thanks, @ashutoshcipher. LGTM.
Since majority of boolean configuration properties are suffixed with .enabled, mapreduce.task.ping-for-liveliness-check.enabled might be better than mapreduce.task.enable.ping-for-liveliness-check. You can update before merging if you like.
…ask.ping-for-liveliness-check.enabled
Contributor
Author
|
Thanks @iwasakims for your review and approval. I have update it to |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
iwasakims
pushed a commit
that referenced
this pull request
Jun 20, 2022
…e on MultipleOutputs#close (#4247) Contributed by Ravuri Sushma sree. Co-authored-by: Ashutosh Gupta <ashugpt@amazon.com> (cherry picked from commit 36c4be8) Conflicts: hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app/src/main/java/org/apache/hadoop/mapred/TaskAttemptListenerImpl.java
Member
|
merged this to trunk and branch-3.3. |
HarshitGupta11
pushed a commit
to HarshitGupta11/hadoop
that referenced
this pull request
Nov 28, 2022
…e on MultipleOutputs#close (apache#4247) Contributed by Ravuri Sushma sree. Co-authored-by: Ashutosh Gupta <ashugpt@amazon.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of PR
Fixed MapReduce tasks timing out when spends more time on MultipleOutputs#close