SPARK-7436: Fixed instantiation of custom recovery mode factory and added tests

[jacek-lewandowski]

No description provided.

[AmplabJenkins]

Merged build triggered.

[AmplabJenkins]

Merged build started.

[SparkQA]

Test build #32102 has started for PR 5977 at commit ff0a3c2.

[SparkQA]

Test build #32102 has finished for PR 5977 at commit ff0a3c2.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[AmplabJenkins]

Merged build finished. Test PASSed.

[AmplabJenkins]

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/32102/
Test PASSed.

[JoshRosen]

@ScrapCodes, could you take a quick look through this since you wrote the original patch that added support for custom recovery mode factories? See https://issues.apache.org/jira/browse/SPARK-7436 for a description of the issue addressed by this patch.

[ScrapCodes]

@JoshRosen Thanks for pointing out.

Until now I was in the impression of x.getClass is same as classOf[X].

scala> class A
defined class A

scala> new A
res0: A = A@2d38eb89

scala> classOf[A]
res1: Class[A] = class A

scala> res0.getClass
res2: Class[_ <: A] = class A

scala> 

Thanks a lot for adding a test suite :). There is minor nit regarding style, that we do not allow infix notations in scala syntax. LGTM otherwise.

[ScrapCodes]

This is what I mentioned about. https://cwiki.apache.org/confluence/display/SPARK/Spark+Code+Style+Guide#SparkCodeStyleGuide-InfixMethods

[jacek-lewandowski]

@ScrapCodes do you mean "should matchers" ?

[ScrapCodes]

Yes.

[jacek-lewandowski]

I don't agree with this - you already use such notation in a few places (for Idea search for \d+\sseconds except comments and string literals shows 16 usages for production code and 71 usages for test code, search for usages of dot-less notation of === method shows 4k+ usages).

Although I can see the benefits of this style guide item for the production code, I cannot understand how it could be forbidden for test code? How would you use "should matchers" without this notation? Scala test frameworks are designed to use dot-less notation to improve readability and understanding of test cases, as well as to decrease unneeded verbosity.

[ScrapCodes]

I am sorry, I was in an assumption we are religious about it. A quick search across the codebase proved my wrong. Matchers are widely used in at least recently added code. I personally agree with your opinion on scala and makes sense for dsl, but then it was just a convention.

@JoshRosen if you are okay, I can go ahead and merge this ?

[ScrapCodes]

@jacek-lewandowski Can you please close other Pull requests, with same issue id and you think are irrelevant.

[jacek-lewandowski]

Thanks @ScrapCodes :)
Other pull requests with the same issue id are against branch-1.3 and branch-1.4. Isn't going to be applied to those branches?

[ScrapCodes]

ahh makes sense then. Thanks a lot @jacek-lewandowski !

[ScrapCodes]

Side Note: Unless it is a non trivial merge, our merge scripts takes care of merging to other branches automatically. So we don't need separate PRs for same patch with no conflicts at all.

[jacek-lewandowski]

@ScrapCodes so this one is enough then?

[ScrapCodes]

Yes, if you think they are all exact same patch.

[jacek-lewandowski]

Ahh, i do remember now - i needed to change some thing in the test while creating branch for some release... so i would keep them.

[JoshRosen]

Thanks for looking this over, @ScrapCodes. This looks good to me as well, so I'm going to merge this along with the other backport PRs. ❤️ the fact that this contains more test code than actual changes!