SPARK-3177 (on Master Branch) #2204
Conversation
|
Can one of the admins verify this patch? |
|
I have tested against 2.0.5-alpha, 2.0.6-alpha (yarn-alpha) and 2.3.0-cdh5.0.0 (yarn-stable) |
| @@ -232,6 +233,15 @@ class ClientBaseSuite extends FunSuite with Matchers { | |||
| def getFieldValue[A, B](clazz: Class[_], field: String, defaults: => B)(mapTo: A => B): B = | |||
| Try(clazz.getField(field)).map(_.get(null).asInstanceOf[A]).toOption.map(mapTo).getOrElse(defaults) | |||
|
|
|||
| def getFieldValue2[A: ClassTag, A1: ClassTag, B](clazz: Class[_], field: String, | |||
There was a problem hiding this comment.
without ClassTag, you will get the following warnings:
abstract type pattern A is unchecked since it is eliminated by erasure
abstract type pattern A1 is unchecked since it is eliminated by erasure
so that
case v: A => mapTo(v)
case v1: A1 => mapTo1(v1)
will not match
with ClassTag, the runtime type are returned and above match will work as expected
There was a problem hiding this comment.
Can you elaborate this ? What's not true ?
Remove the classTag and run the unit test and see yourself.
Sent from my iPhone
On Sep 2, 2014, at 11:00 PM, Prashant Sharma notifications@github.com wrote:
In yarn/common/src/test/scala/org/apache/spark/deploy/yarn/ClientBaseSuite.scala:
@@ -232,6 +233,15 @@ class ClientBaseSuite extends FunSuite with Matchers {
def getFieldValue[A, B](clazz: Class[], field: String, defaults: => B)(mapTo: A => B): B =
Try(clazz.getField(field)).map(.get(null).asInstanceOf[A]).toOption.map(mapTo).getOrElse(defaults)
- def getFieldValue2[A: ClassTag, A1: ClassTag, B](clazz: Class[_], field: String,
This is not true.—
Reply to this email directly or view it on GitHub.
|
Can one of the admins verify this patch? |
|
ok to test |
|
QA tests have started for PR 2204 at commit
|
|
QA tests have finished for PR 2204 at commit
|
|
The unit test failed sql/test:test the changes is for yarn unit test, it doesn't affect the sql/test. On Tue, Sep 9, 2014 at 10:44 AM, Apache Spark QA notifications@github.com
|
| @@ -232,6 +233,15 @@ class ClientBaseSuite extends FunSuite with Matchers { | |||
| def getFieldValue[A, B](clazz: Class[_], field: String, defaults: => B)(mapTo: A => B): B = | |||
| Try(clazz.getField(field)).map(_.get(null).asInstanceOf[A]).toOption.map(mapTo).getOrElse(defaults) | |||
|
|
|||
| def getFieldValue2[A: ClassTag, A1: ClassTag, B](clazz: Class[_], field: String, | |||
| defaults: => B) | |||
| (mapTo: A => B)(mapTo1: A1 => B) : B = | |||
There was a problem hiding this comment.
Can you structure this as follows:
def getFieldValue[...](
arg1: String,
arg2: String,
...)(mapTo: A => B)(mapTo1: A1 => B): B = {
Try(...)
}
There was a problem hiding this comment.
(the style in the rest of this file is not a great example to follow!)
… certain class fields. In this particular case, the ClientBase using reflection to to address this issue, and we need to different way to test the ClientBase's method. Original ClientBaseSuite using getFieldValue() method to do this. But it doesn't work for yarn-alpha as the API returns an array of String instead of just String (which is the case for Yarn-stable API).
To fix the test, I add a new method
def getFieldValue2[A: ClassTag, A1: ClassTag, B](
clazz: Class[_],
field: String,
defaults: => B) (mapTo: A => B)(mapTo1: A1 => B) : B =
Try(clazz.getField(field)).map(_.get(null)).map {
case v: A => mapTo(v)
case v1: A1 => mapTo1(v1)
case _ => defaults
}.toOption.getOrElse(defaults)
to handle the cases where the field type can be either type A or A1. In this new method the type A or A1 is pattern matched and corresponding mapTo function (mapTo or mapTo1) is used.
Reformat based on Andrew's request
|
Reformatted based on Andrew's comment #2204 |
|
QA tests have started for PR 2204 at commit
|
|
QA tests have finished for PR 2204 at commit
|
|
looks good. I tested it on hadoop 0.23 and it works also. |
The JIRA and PR was original created for branch-1.1, and move to master branch now.
Chester
The Issue is due to that yarn-alpha and yarn have different APIs for certain class fields. In this particular case, the ClientBase using reflection to to address this issue, and we need to different way to test the ClientBase's method. Original ClientBaseSuite using getFieldValue() method to do this. But it doesn't work for yarn-alpha as the API returns an array of String instead of just String (which is the case for Yarn-stable API).
To fix the test, I add a new method
def getFieldValue2[A: ClassTag, A1: ClassTag, B](clazz: Class[], field: String,
defaults: => B)
(mapTo: A => B)(mapTo1: A1 => B) : B =
Try(clazz.getField(field)).map(.get(null)).map {
case v: A => mapTo(v)
case v1: A1 => mapTo1(v1)
case _ => defaults
}.toOption.getOrElse(defaults)
to handle the cases where the field type can be either type A or A1. In this new method the type A or A1 is pattern matched and corresponding mapTo function (mapTo or mapTo1) is used.