-
Notifications
You must be signed in to change notification settings - Fork 28.7k
SPARK-4447. Remove layers of abstraction in YARN code no longer needed after dropping yarn-alpha #3652
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
Conversation
@@ -17,7 +17,8 @@ | |||
|
|||
package org.apache.spark.deploy.yarn | |||
|
|||
import java.util.{List => JList} | |||
import com.google.common.util.concurrent.ThreadFactoryBuilder |
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.
nit: should go after java and scala imports.
Test build #24273 has started for PR 3652 at commit
|
import org.apache.spark.scheduler.SplitInfo | ||
import org.apache.spark.util.Utils | ||
|
||
import scala.collection.JavaConversions._ |
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.
nit: should be right after java imports.
I assume most here is just code motion, so I didn't pay that close attention to the changes in the container allocation code. If that's true, then this LGTM. |
Test build #24273 has finished for PR 3652 at commit
|
Test PASSed. |
@@ -51,13 +53,14 @@ object AllocationType extends Enumeration { | |||
// Refer to http://developer.yahoo.com/blogs/hadoop/posts/2011/03/mapreduce-nextgen-scheduler/ for | |||
// more info on how we are requesting for containers. | |||
|
|||
|
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.
delete
This looks fine on a first glance. I plan to take a detailed pass in the next day or two to make sure we didn't change any logic. |
Just took a deeper look comparing the old code with the new code and couldn't find any glaring problems. I'll test and merge this once you address the comments. |
17628f8
to
2791158
Compare
Cool, updated patch reflects Marcelo and Andrew's comments. |
Test build #24676 has started for PR 3652 at commit
|
Test build #24676 has finished for PR 3652 at commit
|
Test PASSed. |
No description provided.