You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by chesterxgchen <gi...@git.apache.org> on 2014/08/29 20:50:51 UTC
[GitHub] spark pull request: SPARK-3177 (on Master Branch)
GitHub user chesterxgchen opened a pull request:
https://github.com/apache/spark/pull/2204
SPARK-3177 (on Master Branch)
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.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/AlpineNow/spark SPARK-3177-master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/2204.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #2204
----
commit 77be5d0640f8a61e525e6ef8b59fc202b14e7798
Author: chesterxgchen <ch...@alpinenow.com>
Date: 2014-08-29T18:48:51Z
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.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-3177 (on Master Branch)
Posted by chesterxgchen <gi...@git.apache.org>.
Github user chesterxgchen commented on the pull request:
https://github.com/apache/spark/pull/2204#issuecomment-55013602
The unit test failed *sql/test:test*
I run it locally, with the same configuration, the test passed.
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 <no...@github.com>
wrote:
> QA tests have finished
> <https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/20046/consoleFull>
> for PR 2204 at commit 77be5d0
> <https://github.com/apache/spark/commit/77be5d0640f8a61e525e6ef8b59fc202b14e7798>
> .
>
> - This patch *fails* unit tests.
> - This patch merges cleanly.
> - This patch adds no public classes.
>
> —
> Reply to this email directly or view it on GitHub
> <https://github.com/apache/spark/pull/2204#issuecomment-55007803>.
>
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-3177 (on Master Branch)
Posted by chesterxgchen <gi...@git.apache.org>.
Github user chesterxgchen commented on a diff in the pull request:
https://github.com/apache/spark/pull/2204#discussion_r16964789
--- Diff: 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,
--- End diff --
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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-3177 (on Master Branch)
Posted by chesterxgchen <gi...@git.apache.org>.
Github user chesterxgchen commented on the pull request:
https://github.com/apache/spark/pull/2204#issuecomment-53916231
I have tested against 2.0.5-alpha, 2.0.6-alpha (yarn-alpha) and 2.3.0-cdh5.0.0 (yarn-stable)
all yarn-alpha/test and yarn-stable/test are passed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-3177 (on Master Branch)
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/2204#issuecomment-54996816
ok to test
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-3177 (on Master Branch)
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/2204
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-3177 (on Master Branch)
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2204#issuecomment-55547575
[QA tests have finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/20314/consoleFull) for PR 2204 at commit [`e72a6ea`](https://github.com/apache/spark/commit/e72a6eab54d2b93d09a18b33670f398411cd17a1).
* This patch **passes** unit tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-3177 (on Master Branch)
Posted by chesterxgchen <gi...@git.apache.org>.
Github user chesterxgchen commented on the pull request:
https://github.com/apache/spark/pull/2204#issuecomment-55545571
Reformatted based on Andrew's comment #2204
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-3177 (on Master Branch)
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/2204#discussion_r17331783
--- Diff: 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,
+ defaults: => B)
+ (mapTo: A => B)(mapTo1: A1 => B) : B =
--- End diff --
(the style in the rest of this file is not a great example to follow!)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-3177 (on Master Branch)
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2204#issuecomment-55007803
[QA tests have finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/20046/consoleFull) for PR 2204 at commit [`77be5d0`](https://github.com/apache/spark/commit/77be5d0640f8a61e525e6ef8b59fc202b14e7798).
* This patch **fails** unit tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-3177 (on Master Branch)
Posted by ScrapCodes <gi...@git.apache.org>.
Github user ScrapCodes commented on a diff in the pull request:
https://github.com/apache/spark/pull/2204#discussion_r17032605
--- Diff: 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,
--- End diff --
This is not true.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-3177 (on Master Branch)
Posted by ScrapCodes <gi...@git.apache.org>.
Github user ScrapCodes commented on a diff in the pull request:
https://github.com/apache/spark/pull/2204#discussion_r16945967
--- Diff: 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,
--- End diff --
Why do you need `ClassTag` ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-3177 (on Master Branch)
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2204#issuecomment-55545608
[QA tests have started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/20314/consoleFull) for PR 2204 at commit [`e72a6ea`](https://github.com/apache/spark/commit/e72a6eab54d2b93d09a18b33670f398411cd17a1).
* This patch merges cleanly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-3177 (on Master Branch)
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/2204#discussion_r17331761
--- Diff: 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,
+ defaults: => B)
+ (mapTo: A => B)(mapTo1: A1 => B) : B =
--- End diff --
Can you structure this as follows:
```
def getFieldValue[...](
arg1: String,
arg2: String,
...)(mapTo: A => B)(mapTo1: A1 => B): B = {
Try(...)
}
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-3177 (on Master Branch)
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on the pull request:
https://github.com/apache/spark/pull/2204#issuecomment-55910777
looks good. I tested it on hadoop 0.23 and it works also.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-3177 (on Master Branch)
Posted by chesterxgchen <gi...@git.apache.org>.
Github user chesterxgchen commented on a diff in the pull request:
https://github.com/apache/spark/pull/2204#discussion_r17048232
--- Diff: 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,
--- End diff --
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 <no...@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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-3177 (on Master Branch)
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/2204#issuecomment-53916073
Can one of the admins verify this patch?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-3177 (on Master Branch)
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/2204#issuecomment-55784185
@tgravescs
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-3177 (on Master Branch)
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2204#issuecomment-55000338
[QA tests have started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/20046/consoleFull) for PR 2204 at commit [`77be5d0`](https://github.com/apache/spark/commit/77be5d0640f8a61e525e6ef8b59fc202b14e7798).
* This patch merges cleanly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-3177 (on Master Branch)
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2204#issuecomment-54694372
Can one of the admins verify this patch?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org